Skip to content

Conversation

ada4a
Copy link

@ada4a ada4a commented Sep 14, 2025

For #1950

This turned out rather big... I'd like to make this easier to review, but I'm not sure how -- maybe split each lint's changes into its own commit?

@Urgau
Copy link
Member

Urgau commented Sep 14, 2025

This turned out rather big... I'd like to make this easier to review, but I'm not sure how -- maybe split each lint's changes into its own commit?

If you can split into smaller commits, that would be great. Not necessarily each lint into it's own commit, bundling multiple lints changes is fine.

@ada4a
Copy link
Author

ada4a commented Sep 21, 2025

I did try to group related lints, but there were many small commits left from lints that I couldn't find a good group for -- hopefully, having the lint name on those commits will at least make their intent clear.

@Urgau
Copy link
Member

Urgau commented Sep 21, 2025

Thanks for splitting into many commits, that will help a lot.

I will take a look at the PR in the next few weeks (I'm on vacation for the next two weeks).

Copy link
Member

@Urgau Urgau left a comment

Choose a reason for hiding this comment

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

This looks pretty good. Thanks for doing that.

Left some changes and a few nits.

View changes since this review

@ada4a
Copy link
Author

ada4a commented Oct 9, 2025

All the review comments should be addressed now, and the PR rebased on latest master

A range-diff could've helped here 😅

@Urgau
Copy link
Member

Urgau commented Oct 9, 2025

A range-diff could've helped here 😅

Fortunately it's possible to ask triagebot nicely and it will give one nicely. :-)

Copy link
Member

@Urgau Urgau 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 to me. Will approve and merge in a few days unless someone (cc @Kobzol) wants to take a look before that.

View changes since this review

@Urgau
Copy link
Member

Urgau commented Oct 9, 2025

Btw, it seems like my PR #2194 created a conflict with yours (sorry about that), could you rebase on master once again.

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