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

Use a stricter configuration for basedpyright #332

Merged
merged 26 commits into from
Jan 7, 2025
Merged

Conversation

ItsDrike
Copy link
Member

@ItsDrike ItsDrike commented Jul 12, 2024

A type checker that isn't strict is a bad type checker

This moves us from the previously used typeCheckingMode, set as standard to using all, with some explicity disabled diagnostics, which we don't want to enforce (blacklist model). This is generally a better idea, as it will also auto-enable any new rules introduced into basedpyright even if they aren't enabled in the mode we're using by default, allowing us to make the conscious decision of whether or not we want to enable it.

Warning

This will likely break all PRs :P

Note that this is the rare case where if you wish to bring your PR up to date with main, you will want to do a proper git merge as opposed to git rebase, so that you can bring your code up-to-date with these changes in that merge commit, rather than having to edit each commit in a rebase to be compatible. Unless you only have very few commits (1-3), or your commits don't change code a lot (documentation PRs).

This is because a rebase would require you to get your code added after these commits will have already been made, which means you will likely encounter git conflicts on every single commit (and even if it won't straight up be git conflicts, your own new code will likely no longer be valid for that commit, as it won't be passing the new stricter type-checker configuration), forcing you to edit that commit. With a lot of commits, you will end up having to edit pretty much all of them one by one, while with a proper merge, you can simply fix the type issues in a single merge commit.

@ItsDrike ItsDrike added t: enhancement Improvement to an existing feature p: 2 - normal Normal priority a: internal Related to internal API of the project labels Jul 12, 2024
@ItsDrike ItsDrike force-pushed the strict-basedpyright branch 3 times, most recently from ccc29b0 to 45c4b47 Compare July 12, 2024 10:39
ItsDrike added a commit that referenced this pull request Jul 12, 2024
ItsDrike added a commit that referenced this pull request Jul 12, 2024
@ItsDrike ItsDrike force-pushed the strict-basedpyright branch 4 times, most recently from bfbdfb6 to 66c989c Compare July 12, 2024 13:37
@ItsDrike ItsDrike force-pushed the strict-basedpyright branch from 2a350b8 to 347dd59 Compare July 13, 2024 09:29
@ItsDrike ItsDrike marked this pull request as ready for review July 13, 2024 10:06
@ItsDrike
Copy link
Member Author

ItsDrike commented Jul 13, 2024

Note: Unknown type reporting is currently disabled, there is a comment in pyproject.toml where these are disabled mentioning that we should consider enabling them. This will however not be done in this PR, instead, it will be covered by a future PR (#333), as I'm not entirely certain if enabling these is a good idea for us, they introduce a LOT of work, and while they can definitely be beneficial, I'm not sure it's worth the annoyance that they bring. I don't really wish to discuss the pros/cons of this in this PR, that discussion should take place in the PR that attempts to enable them.

Note 2: I've added some TODO comments into the code-base, which were not actually resolved here. That's because these TODOs are completely unrelated to this PR and are essentially just issues that I've noticed while working on this. These should be fixed in another PR(s) some time in the future.

@py-mine-ci-bot py-mine-ci-bot bot added the s: stale Has had no activity for a while (will be closed for inactivity/marked up for grabs soon) label Nov 22, 2024
@ItsDrike ItsDrike removed the s: stale Has had no activity for a while (will be closed for inactivity/marked up for grabs soon) label Jan 7, 2025
@ItsDrike ItsDrike marked this pull request as draft January 7, 2025 01:01
@ItsDrike
Copy link
Member Author

ItsDrike commented Jan 7, 2025

Marking this as draft again, pending the consideration of enabling reportUnannotatedClassAttribute. This rule is new and didn't exist at the time this PR was made. I disabled it during the merge with main that bumped pyright, because this rule triggered a LOT of diagnostics, and I haven't yet decided whether it'd be a right choice for us.

@ItsDrike ItsDrike marked this pull request as ready for review January 7, 2025 21:57
@ItsDrike
Copy link
Member Author

ItsDrike commented Jan 7, 2025

Marking as ready again: reportUnannotatedClassAttribute rule will not be enabled for mcproto, it's too annoying without that many benefits.

@ItsDrike ItsDrike merged commit 20b0903 into main Jan 7, 2025
10 checks passed
@ItsDrike ItsDrike deleted the strict-basedpyright branch January 7, 2025 22:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a: internal Related to internal API of the project p: 2 - normal Normal priority t: enhancement Improvement to an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant