chore: generic change to reduce the number of compilation warnings #2696

Ivansete-status · 2024-05-13T16:51:10Z

Description

Simple but long (sorry for that) PR that aims to reduce the number of compilation warnings.

Compilation warnings related to "imported module but not used".
Changes around async: (raises:[..]) pragma
re is deprecated
Make proc's not to raise exceptions. This is a little bit beyond the original purpose of the PR. I can submit it in a separate one if you consider appropriate.

github-actions · 2024-05-13T21:32:16Z

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2696-rln-v1

Built from dc59888

github-actions · 2024-05-13T21:32:48Z

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2696-rln-v2

Built from dc59888

gabrielmer

Oh that's reaally nice! Thanks so much!

Just added a couple questions

gabrielmer · 2024-05-15T12:16:36Z

waku/waku_filter_v2/protocol.nim

@@ -312,13 +311,13 @@ proc startMaintainingSubscriptions(wf: WakuFilter, interval: Duration) =

  wf.maintenanceTask = setTimer(Moment.fromNow(interval), maintainSubs)

-method start*(wf: WakuFilter) {.async.} =
+method start*(wf: WakuFilter) {.async, base.} =


just to understand, why do we need the base pragma?

For proper inheritance I think?

Thanks for the comments @gabrielmer , @SionoiS !
This change actually comes from a compiler warning, where it stated that it was deprecated to use non-base methods (I can't remember the precise message.)

gabrielmer · 2024-05-15T12:20:24Z

waku/waku_filter_v2/client.nim

+  try:
+    await connection.writeLP(filterSubscribeRequest.encode().buffer)
+
+    let respBuf = await connection.readLp(DefaultMaxSubscribeResponseSize)
+    let respDecodeRes = FilterSubscribeResponse.decode(respBuf)
+    if respDecodeRes.isErr():
+      trace "Failed to decode filter subscribe response", servicePeer
+      waku_filter_errors.inc(labelValues = [decodeRpcFailure])
+      return err(FilterSubscribeError.badResponse(decodeRpcFailure))
+
+    let response = respDecodeRes.get()
+
+    if response.requestId != filterSubscribeRequest.requestId:
+      trace "Filter subscribe response requestId mismatch", servicePeer, response
+      waku_filter_errors.inc(labelValues = [requestIdMismatch])
+      return err(FilterSubscribeError.badResponse(requestIdMismatch))
+
+    if response.statusCode != 200:
+      trace "Filter subscribe error response", servicePeer, response
+      waku_filter_errors.inc(labelValues = [errorResponse])
+      let cause =
+        if response.statusDesc.isSome():
+          response.statusDesc.get()
+        else:
+          "filter subscribe error"
+      return err(FilterSubscribeError.parse(response.statusCode, cause = cause))
+  except CatchableError:
+    let errMsg = "exception in waku_filter_v2 client: " & getCurrentExceptionMsg()
+    trace "exception in waku_filter_v2 client", error = getCurrentExceptionMsg()
+    waku_filter_errors.inc(labelValues = [errMsg])
+    return err(FilterSubscribeError.badResponse(errMsg))


wouldn't it be better for the try block to be more scoped?

wouldn't it be better for the try block to be more scoped?

Yes, you are absolutely right! I tried to apply that in 06546bb

SionoiS

Such a good clean-up. Amazing!

SionoiS · 2024-05-15T12:24:26Z

tests/waku_store_legacy/test_wakunode_store.nim

@@ -31,7 +31,7 @@ import
  ../testlib/wakucore,
  ../testlib/wakunode

-procSuite "WakuNode - Store":
+procSuite "WakuNode - Store Legacy":


ah thanks I totally forgot about renaming tests!

SionoiS · 2024-05-15T12:27:20Z

waku/node/waku_node.nim

+    try:
+      await node.wakuFilter.start()
+    except CatchableError:
+      error "failed to start wakuFilter", error = getCurrentExceptionMsg()


You don't like the catch: macro?

Suggested change

try:

await node.wakuFilter.start()

except CatchableError:

error "failed to start wakuFilter", error = getCurrentExceptionMsg()

let catchable = catch: await node.wakuFilter.start()

if catchable.isErr():

error "failed to start wakuFilter", error = catchable.error.msg

@SionoiS We would need better a valueOrCatch macro... hm, due I don't think catch works together with valueOr template.

@SionoiS We would need better a valueOrCatch macro... hm, due I don't think catch works together with valueOr template.

Yes it's unfortunate, otherwise this would be possible

(catch: await node.wakuFilter.start()).isOkOr: error "failed to start wakuFilter", error = error.msg

@SionoiS We would need better a valueOrCatch macro... hm, due I don't think catch works together with valueOr template.

Yes it's unfortunate, otherwise this would be possible

(catch: await node.wakuFilter.start()).isOkOr: error "failed to start wakuFilter", error = error.msg

Well we can add! I may do it somewhere.

You don't like the catch: macro?

Well, I "don't like" maybe because I don't fully understand it xD
To me, it sounds a bit more verbose the traditional approach but I'll give it another glance ;P

SionoiS · 2024-05-15T12:32:00Z

waku/waku_filter_v2/protocol.nim

@@ -312,13 +311,13 @@ proc startMaintainingSubscriptions(wf: WakuFilter, interval: Duration) =

  wf.maintenanceTask = setTimer(Moment.fromNow(interval), maintainSubs)

-method start*(wf: WakuFilter) {.async.} =
+method start*(wf: WakuFilter) {.async, base.} =


For proper inheritance I think?

NagyZoltanPeter

This is Spartaaa!!!! ... I would say so after our greek tour, but I better say: It is amazing and very needed PR. Special thank for the depricate net lib warning elimination!

NagyZoltanPeter · 2024-05-15T15:09:39Z

waku/node/waku_node.nim

+    try:
+      await node.wakuFilter.start()
+    except CatchableError:
+      error "failed to start wakuFilter", error = getCurrentExceptionMsg()


@SionoiS We would need better a valueOrCatch macro... hm, due I don't think catch works together with valueOr template.

github-actions bot assigned Ivansete-status May 13, 2024

Ivansete-status force-pushed the avoid-compilation-warnings branch 3 times, most recently from 0735440 to c2a6bb3 Compare May 15, 2024 10:37

Ivansete-status marked this pull request as ready for review May 15, 2024 11:20

Ivansete-status requested review from SionoiS, gabrielmer and NagyZoltanPeter May 15, 2024 11:20

darshankabariya self-requested a review May 15, 2024 11:23

gabrielmer approved these changes May 15, 2024

View reviewed changes

SionoiS approved these changes May 15, 2024

View reviewed changes

NagyZoltanPeter approved these changes May 15, 2024

View reviewed changes

Ivansete-status added 7 commits May 16, 2024 18:21

Generic change to reduce the number of compilation warnings

036b6f9

group_manager.nim: restore back import that is needed

23f0cba

utils/collector.nim: enforce not raising exception

e17abea

waku.nim: re-adding needed import

25339bf

test_app: add waku_metrics import

023e6cf

test_wakunode_store: make it clear that this file belongs to legacy

4c5dfa8

waku_filter_v2/client.nim: narrow down exception scope

13921f1

Ivansete-status force-pushed the avoid-compilation-warnings branch from 06546bb to 13921f1 Compare May 16, 2024 16:21

Ivansete-status merged commit 78132dc into master May 16, 2024
14 of 15 checks passed

Ivansete-status deleted the avoid-compilation-warnings branch May 16, 2024 20:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: generic change to reduce the number of compilation warnings #2696

chore: generic change to reduce the number of compilation warnings #2696

Ivansete-status commented May 13, 2024

github-actions bot commented May 13, 2024 •

edited

Loading

github-actions bot commented May 13, 2024 •

edited

Loading

gabrielmer left a comment

gabrielmer May 15, 2024

SionoiS May 15, 2024

Ivansete-status May 15, 2024

gabrielmer May 15, 2024

Ivansete-status May 16, 2024

SionoiS left a comment

SionoiS May 15, 2024

SionoiS May 15, 2024

NagyZoltanPeter May 15, 2024

SionoiS May 15, 2024

NagyZoltanPeter May 15, 2024

Ivansete-status May 16, 2024

SionoiS May 15, 2024

NagyZoltanPeter left a comment

NagyZoltanPeter May 15, 2024

chore: generic change to reduce the number of compilation warnings #2696

chore: generic change to reduce the number of compilation warnings #2696

Conversation

Ivansete-status commented May 13, 2024

Description

github-actions bot commented May 13, 2024 • edited Loading

github-actions bot commented May 13, 2024 • edited Loading

gabrielmer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SionoiS left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NagyZoltanPeter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 13, 2024 •

edited

Loading

github-actions bot commented May 13, 2024 •

edited

Loading