Skip to content
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

feat: add lint struct_with_no_pub_fields_changed #962

Merged

Conversation

CommanderStorm
Copy link
Contributor

@CommanderStorm CommanderStorm commented Oct 5, 2024

This draft is mostly to prevent others thinking that nobody is working on this issue.

The query is mostly from struct_with_pub_fields_changed

There is still some tinkering I need to do with

Resolves one checkbox of #954

@CommanderStorm CommanderStorm changed the title intial draft of struct_with_no_pub_fields_changed feat: intial draft of struct_with_no_pub_fields_changed Oct 5, 2024
@CommanderStorm CommanderStorm marked this pull request as ready for review October 6, 2024 13:26
@CommanderStorm
Copy link
Contributor Author

(idk if you get notifications for this, assuming you don't)

@obi1kenobi I have reviewed this as thoroughly as possible. Except for the comments above I have not found anything. => I think this is ready for review

@CommanderStorm CommanderStorm changed the title feat: intial draft of struct_with_no_pub_fields_changed feat: added lint struct_with_no_pub_fields_changed Oct 6, 2024
Copy link
Owner

@obi1kenobi obi1kenobi left a comment

Choose a reason for hiding this comment

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

Nice work! Especially good job with the query logic, it was trickier than I thought it would be.

There are a handful of remaining items that require another pass before we can merge. Most of them are the kinds of things that would be good to practice catching in your self-reviews: the top difference between a junior engineer and a senior engineer is whether one independently finds the issues in one's own code, or whether someone else has to point them out. So let's work on it together!

src/lints/struct_with_no_pub_fields_changed.ron Outdated Show resolved Hide resolved
src/lints/struct_with_no_pub_fields_changed.ron Outdated Show resolved Hide resolved
src/lints/struct_with_no_pub_fields_changed.ron Outdated Show resolved Hide resolved
src/lints/struct_with_no_pub_fields_changed.ron Outdated Show resolved Hide resolved
Copy link
Owner

@obi1kenobi obi1kenobi left a comment

Choose a reason for hiding this comment

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

Looks good modulo a small bug in the witness hint. If you can regenerate the witness file with the fix, this should be good to merge!

src/lints/struct_with_no_pub_fields_changed_type.ron Outdated Show resolved Hide resolved
@CommanderStorm CommanderStorm changed the title feat: added lint struct_with_no_pub_fields_changed feat: add lint struct_with_no_pub_fields_changed Oct 6, 2024
Copy link
Owner

@obi1kenobi obi1kenobi left a comment

Choose a reason for hiding this comment

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

Awesome, nicely done 🚀

@obi1kenobi obi1kenobi merged commit cd90fdc into obi1kenobi:main Oct 6, 2024
34 checks passed
@CommanderStorm CommanderStorm deleted the struct_without_pub_fields_changed branch October 7, 2024 00:34
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.

3 participants