Skip to content

Conversation

djc
Copy link
Contributor

@djc djc commented Oct 11, 2025

@djc djc requested review from ChrisDenton and rami3l October 11, 2025 11:48
@rami3l rami3l self-assigned this Oct 11, 2025
Copy link
Member

@rami3l rami3l left a comment

Choose a reason for hiding this comment

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

I've reviewed all the stuff up to and including 9182627 and it looks mostly great to me modulo some minor questions.

Should I continue reviewing or wait for the CI to pass?

Anyway, many thanks for simplifying this much of the code!

@djc
Copy link
Contributor Author

djc commented Oct 11, 2025

I've reviewed all the stuff up to and including 9182627 and it looks mostly great to me modulo some minor questions.

Thanks for the quick response!

Should I continue reviewing or wait for the CI to pass?

I don't expect to make any major changes -- having all feedback before I address your feedback would be useful, if you have time.

@rami3l
Copy link
Member

rami3l commented Oct 12, 2025

I don't expect to make any major changes -- having all feedback before I address your feedback would be useful, if you have time.

@djc I just checked and can confirm that I have no more questions with the rest of the commits then.

@djc djc force-pushed the notifications-4 branch from 354710a to 3f3b3ba Compare October 12, 2025 08:38
@djc djc force-pushed the notifications-4 branch from 3f3b3ba to 029ac91 Compare October 12, 2025 12:52
@djc djc force-pushed the notifications-4 branch from 029ac91 to 552ade9 Compare October 12, 2025 13:21
@djc
Copy link
Contributor Author

djc commented Oct 12, 2025

  • Addressed feedback
  • Added a commit, "cli: build Cfg earlier in setup mode"
  • Fixed Clippy issues and Windows compile errors

@rami3l
Copy link
Member

rami3l commented Oct 12, 2025

@djc Many thanks for the effort!

@FranciscoTGouveia has promised to take it from here :)

@djc djc added this pull request to the merge queue Oct 12, 2025
Merged via the queue into main with commit 04dd819 Oct 12, 2025
29 checks passed
@djc djc deleted the notifications-4 branch October 12, 2025 15:55
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.

2 participants