chore: refactor relative path to better absolute #2861
Conversation
|
You can find the image built from this PR at Built from 6332392 |
|
This PR may contain changes to configuration options of one of the apps. If you are introducing a breaking change (i.e. the set of options in latest release would no longer be applicable) make sure the original option is preserved with a deprecation note for 2 following releases before it is actually removed. Please also make sure the label |
darshankabariya
requested review from
NagyZoltanPeter,
gabrielmer and
Ivansete-status
There was a problem hiding this comment.
In general I like the approach, but I would better leave the reference to the waku/ directory and just eliminate the ../.. like stuff.
I think the would help reading the imports. Similarly we have chronos/... nim-eth/, etc references in imports.
So treat waku as a lib for tests and apps. WDYT?
apps/chat2bridge/nim.cfg
Outdated
| @@ -1,3 +1,4 @@ | |||
| -d:chronicles_line_numbers | |||
| -d:chronicles_runtime_filtering:on | |||
| -d:discv5_protocol_id:d5waku | |||
| path = "../../waku" | |||
There was a problem hiding this comment.
Me, personally would prefer better to always use the waku/ prefix all the time we refer the library from an app.
For me it is harder to read from where are those imports coming.
WDYT?
|
@darshankabariya: Please consider to wait with merging it with in sync with next release, maybe just before cut off and release-notes. That will help other's branch rebasing before merge, otherwise there can be many conflicts for some branches. |
Got it ! it's better clarity then this. |
Thank you for the suggestion. I understand the concern regarding potential conflicts. All the changes are in the tests and apps directories, tests directory shouldn't cause i guess. However, I agree that waiting a little bit before merging could be beneficial. |
There was a problem hiding this comment.
It is looking beautiful, thanks so much for it! 🙌
Notice that we need to perform some minor adjustments to the tests so that the Ubuntu tests pass as well :P
I agree with you @NagyZoltanPeter that we need to wait until the v0.30.0 is released. @darshankabariya , you will need to rebase from master after the v0.30.0 is released.
On the other hand, we still need to push it a bit further as there are some pending files to be reviewed.
We need the following commands to return 0 (maybe we can add them as a "lint" check on every PR:)
find nwaku/ -mindepth 2 -maxdepth 2 -type f -name '*.nim' -print0 | xargs -0 grep -Fn ../.. | wc -l
find nwaku/ -mindepth 3 -maxdepth 3 -type f -name '*.nim' -print0 | xargs -0 grep -Fn ../../.. | wc -l
find nwaku/ -mindepth 4 -maxdepth 4 -type f -name '*.nim' -print0 | xargs -0 grep -Fn ../../../.. | wc -l
| let | ||
| msgOverLimit = fakeWakuMessage( | ||
| contentTopic = contentTopic, | ||
| payload = getByteSequence(DefaultMaxWakuMessageSize + 64 * 1024), | ||
| ) | ||
|
|
There was a problem hiding this comment.
That change sounds as use of different nph version. Maybe is sth we already had. Make sure you applied nph 0.5.1 to that file :)
There was a problem hiding this comment.
i am using that version only. If you want the original state, I will disable it for a while to revert to the previous state.
| @@ -12,8 +12,7 @@ import | |||
| from std/times import epochTime | |||
|
|
|||
| import | |||
| ../../../waku/ | |||
| [node/waku_node, node/peer_manager, waku_core, waku_node, waku_rln_relay], | |||
| waku/node/waku_node, waku/node/peer_manager, waku/waku_core, waku/aku_node, waku/waku_rln_relay | |||
There was a problem hiding this comment.
Tiny typo:
| waku/node/waku_node, waku/node/peer_manager, waku/waku_core, waku/aku_node, waku/waku_rln_relay | |
| waku/node/waku_node, waku/node/peer_manager, waku/waku_core, waku/waku_node, waku/waku_rln_relay |
There was a problem hiding this comment.
Actually, I used a script for these refactor changes. It's open bracket, but I will bundle everything together where necessary and complete the remaining refactoring as well. Thanks for review.
There was a problem hiding this comment.
Hi @Ivansete-status,
Please review the PR one more time. There have been many changes after your approval, it’s taking a bit longer than expected because due to bulk changes trouble with build. Besides the command mentioned in the issue, I also attempted to refactor the extra relative paths in the project which missed by those commands. Just reply this comment so get notified for merge.
|
Thanks so much for that @darshankabariya ! ❤️ |
closes #2845