Skip to content
New issue

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

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

Already on GitHub? # to your account

refactoring to add lint levels on queries #787

Merged
merged 3 commits into from
Jun 4, 2024

Conversation

suaviloquence
Copy link
Contributor

  • split part of check_release::run_check_release into a helper function print_lint_error
  • add LintLevel struct in query and add it as a field to SemverQuery
  • add default Deny field to all existing lints in lints/
  • add shell_error function in GlobalConfig

* split part of `check_release::run_check_release` into a helper
  function `print_lint_error`
* add `LintLevel` struct in `query` and add it as a field to
  `SemverQuery`
* add default `Deny` field to all existing lints in `lints/`
* add `shell_error` function in `GlobalConfig
src/check_release.rs Show resolved Hide resolved
src/check_release.rs 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.

Reverting my suggestions and merging as it was before. There's still a bit of an API design issue around how we do printing, but we can resolve it in the future -- it doesn't have to block us right now.

src/check_release.rs Outdated Show resolved Hide resolved
src/check_release.rs Outdated Show resolved Hide resolved
@obi1kenobi obi1kenobi enabled auto-merge (squash) June 4, 2024 13:52
@obi1kenobi obi1kenobi merged commit a8083aa into obi1kenobi:main Jun 4, 2024
32 checks passed
@obi1kenobi
Copy link
Owner

From a PR mechanics perspective, I'd recommend using even smaller PRs to make them even easier to merge.

This PR does three unrelated things, as you noted by writing its summary as a bulleted list:

  1. split part of check_release::run_check_release into a helper function print_lint_error
  2. add LintLevel struct in query and add it as a field to SemverQuery
    A) also add default Deny field to all existing lints in lints/
  3. add shell_error function in GlobalConfig

None of these three things needed to be in the same PR as the rest, and a 10-line PR is a lot easier to review and merge compared to a 100-line one. So don't worry about opening lots of PRs concurrently for orthogonal functionality. It's fine even if the PRs are tiny — we'll both move much faster that way.

@suaviloquence
Copy link
Contributor Author

Got it, thanks so much for the feedback!

@suaviloquence
Copy link
Contributor Author

On that note, how would you recommend writing the PRs to add the override data structures? Before, there were three: one instance of an override (called QueryOverride), the map of lint names to QueryOverride structs, and the stack of those maps. My intuition is that because they are related, they should all go in the same PR, but I have been making too-big PRs lately hehe. I could make three, one for each added data structure, but they are dependent on each other, so I'm not sure if that would work. Thanks!

@obi1kenobi
Copy link
Owner

All three of those seem quite related to each other, so it's probably okay to put them in the same PR if you can't figure out a good way to split them into completely orthogonal changes that can stand independently.

Not every PR has to be tiny, but the more you're able to find ways to make them small and self-contained, the faster our velocity and higher our code quality will generally be. This is a skill that one can practice just like any other.

I'd recommend this:

  • Before you open a PR, self-review it in the GitHub interface. Read it over, polish whatever you notice could be improved, and see if you notice things that are unrelated to each other that might be candidates to split it.
  • Then, split out any unrelated changes into separate PRs and repeat there as needed.
  • When you open a PR for my review, I'll also try to point out things that could be polished or split up. Your objective can be to get the number of my suggestions to zero, so that as often as possible, our workflow looks like "you open a PR, I approve and merge, then repeat." 🚀

You're a good engineer, and if you use the same GitHub PR review UI I use, I bet you'll notice 99.9% of the things I'd flag in my review. But it's very important to do the self-review in the GitHub UI, not in your editor. Our brains have a weird tendency to miss details if they are in an environment they are very used to (like if you've been coding a new feature for a few hours), and the shakeup of looking at the code in a new environment will let you see things you otherwise would have missed.

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

Successfully merging this pull request may close these issues.

2 participants