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

chore: enable lints fully #426

Merged
merged 1 commit into from
Oct 23, 2023
Merged

chore: enable lints fully #426

merged 1 commit into from
Oct 23, 2023

Conversation

JP-Ellis
Copy link
Contributor

📝 Summary

As there is a significant existing codebase which will eventually be deprecated, there is little benefit to applying formatting and linting rules to the files which are to be deprecated.

This commit configures Ruff, Mypy and Black to exclude files in the v2 codebase so that hatch run lint can function correctly. As a result, the linting in the CI workflow has also been enabled.

An equivalent was already implemented for the pre-commit hooks.

🚨 Breaking Changes

🔥 Motivation

🔨 Test Plan

🔗 Related issues/PRs

@JP-Ellis JP-Ellis requested a review from YOU54F October 19, 2023 06:56
@JP-Ellis JP-Ellis self-assigned this Oct 19, 2023
@JP-Ellis JP-Ellis enabled auto-merge (rebase) October 22, 2023 22:43
@YOU54F
Copy link
Member

YOU54F commented Oct 23, 2023

Just a single comment above ☝🏾

I'm not sure I am keen on the auto-merge with a single approval, it makes me feel uncomfortable leaving an approved review, and I feel like I would rather just comment. I'd rather the merging be done by the author of the PR. - That is personal preference though, and my own sense of ownership.

also it gets rebased, as part of the merge, would we ever be in a situation where we are testing stale changes that won't reflect the main branch post the merge/rebase. How do we test the post merged state, pre merge.

(I'm not sure I can see how many commits behind the branch is, from the main branch, from the GH ui )

@JP-Ellis
Copy link
Contributor Author

I'm not sure I am keen on the auto-merge with a single approval, it makes me feel uncomfortable leaving an approved review, and I feel like I would rather just comment. I'd rather the merging be done by the author of the PR. - That is personal preference though, and my own sense of ownership.

I typically enable auto-merge when the PR is on the simpler side. I really like having that enabled because it means I don't have to go back and merge everything manually once everything is approved. I don't think there's any harm in you giving a comment and just saying that it is effectively approved, pending on just a minor change.

Are you able to disable the auto-merge in cases like this when you want to approve it?

I think it also depends on how strict we want approvals to be. The strictest measures would require:

  • The PR must be up to the date with the main branch before merge/rebase
  • The approval gets dismissed if there's any change to the PR

I personally don't think these requirements are needed for this project.

also it gets rebased, as part of the merge, would we ever be in a situation where we are testing stale changes that won't reflect the main branch post the merge/rebase. How do we test the post merged state, pre merge.

I personally much prefer a linear Git history without the noise that merge commits introduce, hence why I use rebases.

As for the tests going stale, I think in general it is fine. I can always create a post-merge PR to resolve any issues introduced. As mentioned above, there's a setting on GitHub to require that PRs be up to date with the main branch before merge, which then guarantees that the tests are run against the latest changes. The problem with that is that whenever a PR is merged, all other PRs have to be updated before they can be merged.

As there is a significant existing codebase which will eventually be
deprecated, there is little benefit to applying formatting and linting
rules to the files which are to be deprecated.

This commit configures Ruff, Mypy and Black to exclude files in the v2
codebase so that `hatch run lint` can function correctly. As a result,
the linting in the CI workflow has also been enabled.

An equivalent was already implemented for the pre-commit hooks.

Signed-off-by: JP-Ellis <[email protected]>
@JP-Ellis
Copy link
Contributor Author

I have rebased this PR on top of master and resolved the merge conflicts.

@JP-Ellis JP-Ellis merged commit ab02630 into master Oct 23, 2023
26 of 31 checks passed
@JP-Ellis JP-Ellis deleted the chore/enable-lints-fully branch October 23, 2023 22:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Completed
Development

Successfully merging this pull request may close these issues.

Properly Enable Lints
2 participants