Skip to content

refactor: move parsing into its own module #232

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 25, 2025

Conversation

chuksys
Copy link

@chuksys chuksys commented Mar 16, 2025

Closes #214

@chuksys chuksys changed the title refactor:move parsing into its own module refactor: move parsing into its own module Mar 16, 2025
@chuksys chuksys force-pushed the parsing-mod branch 3 times, most recently from fa1934e to 4ba54fa Compare March 17, 2025 11:49
@chuksys chuksys marked this pull request as ready for review March 17, 2025 11:59
@f3r10
Copy link
Collaborator

f3r10 commented Mar 17, 2025

Also, the standard for the commits is:

Also a small nitpick on commit naming (which is unconventional mostly personal preference, so feel free to ignore), generally we've been using the structure: scope/{action}, eg sim-ln/refactor, sim-cli/test followed by message.

@chuksys chuksys force-pushed the parsing-mod branch 3 times, most recently from c4af86c to 88a6f54 Compare March 17, 2025 15:29
@f3r10
Copy link
Collaborator

f3r10 commented Mar 17, 2025

@chuksys the implementation of async closures also can be done in this PR #231 (comment).
It will be necessary to update the CI version to 1.85 so the CI checks can pass.

@carlaKC
Copy link
Contributor

carlaKC commented Mar 17, 2025

Not sure what's happening with the git history, but 00ffe3f shouldn't be in here - needs a rebase on main.

Happy for us to bump the CI version to take advantage of async closures - let's do that in a preparatory PR and then rebase this one on top of it.

@chuksys
Copy link
Author

chuksys commented Mar 17, 2025

Not sure what's happening with the git history, but 00ffe3f shouldn't be in here - needs a rebase on main.

Happy for us to bump the CI version to take advantage of async closures - let's do that in a preparatory PR and then rebase this one on top of it.

Alright, will open a preparatory PR right away!

@chuksys
Copy link
Author

chuksys commented Mar 17, 2025

Not sure what's happening with the git history, but 00ffe3f shouldn't be in here - needs a rebase on main.
Happy for us to bump the CI version to take advantage of async closures - let's do that in a preparatory PR and then rebase this one on top of it.

Alright, will open a preparatory PR right away!

PR #233

@carlaKC
Copy link
Contributor

carlaKC commented Mar 17, 2025

PR still has an unrelated commit in it: f1af8bb

Also needs to be rebased on #233 to get CI to pass.

@chuksys
Copy link
Author

chuksys commented Mar 17, 2025

PR still has an unrelated commit in it: f1af8bb

Also needs to be rebased on #233 to get CI to pass.

I have removed the unrelated commit while rebasing on top of #233. Please let me know if I got it right and if there are any other fixes I need to make. Thanks.

@carlaKC
Copy link
Contributor

carlaKC commented Mar 18, 2025

There's still a merge commit in this PR and it needs to be rebased on main.

Please take some time to read up in interactive rebases and clean this up properly. If that means things take a bit longer, that's fine. Rather do it once and do it right than require multiple rounds of review on structural issues.

Please also run make check locally before pushing to make sure that clippy + formatting are correct.

@chuksys
Copy link
Author

chuksys commented Mar 18, 2025

There's still a merge commit in this PR and it needs to be rebased on main.

Please take some time to read up in interactive rebases and clean this up properly. If that means things take a bit longer, that's fine. Rather do it once and do it right than require multiple rounds of review on structural issues.

Please also run make check locally before pushing to make sure that clippy + formatting are correct.

Okay will do. Thanks.

Copy link
Collaborator

@f3r10 f3r10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested ACK tested with two cln nodes
image
@chuksys There are only a couple of minor issues then I think it will be ready to merge.

@chuksys chuksys requested a review from f3r10 March 23, 2025 07:38
Copy link
Collaborator

@f3r10 f3r10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested ACK

@chuksys
Copy link
Author

chuksys commented Mar 24, 2025

Thanks so much @f3r10 for taking your time to thoroughly review this PR.

Copy link
Contributor

@carlaKC carlaKC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK, going to go ahead and merge this so that @f3r10 can get started on #215

@@ -13,7 +13,7 @@ jobs:
- name: Install protoc
run: sudo apt install -y protobuf-compiler
- uses: actions/checkout@v4
- uses: dtolnay/rust-toolchain@1.84.0
- uses: dtolnay/rust-toolchain@1.85.0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: for future would prefer for this to be in its own commit.

@carlaKC carlaKC merged commit 2b30a3c into bitcoin-dev-project:main Mar 25, 2025
2 checks passed
@carlaKC carlaKC mentioned this pull request Mar 25, 2025
@chuksys
Copy link
Author

chuksys commented Mar 25, 2025

Thanks for all the feedback @carlaKC. They've been really helpful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor: Move parsing into its own module
3 participants