Skip to content

Refactor checker error messages #10959

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

Merged
merged 15 commits into from
Nov 4, 2021

Conversation

tushar-deepsource
Copy link
Contributor

Description

Migrates checker.py to use the ErrorMessage class.

This is a continuation from the discussion over at #10947

Some notes:

  • Since the type signature of fail cannot be changed altogether, it has been overloaded using a Union to support ErrorMessage objects as well.
  • Some fail calls are still using strings instead of ErrorMessage objects. This is because some of the strings were re-used in other modules, which will be refactored in later PRs.
  • Other PRs will be smaller than this one :)

@TH3CHARLie
Copy link
Collaborator

Can you fix the style error (and also in the other PR)? Then I can review both PRs once all builds are passed.

@tusharsadhwani
Copy link
Contributor

@TH3CHARLie I'm unsure why the mypyc build is failing. For some reason it fails to determine the type of ErrorCode objects being passed inside message_registry.py and I don't know how to fix it. Any clues?

@TH3CHARLie
Copy link
Collaborator

How about providing explicit type like Final[ErrorMessage]?

@tushar-deepsource
Copy link
Contributor Author

Oh wow, so that didn't work but setting the ErrorCode objects as Final[ErrorCode] worked. What?

Anyway, thanks for that.

Copy link
Collaborator

@TH3CHARLie TH3CHARLie left a comment

Choose a reason for hiding this comment

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

Generally LGTM. I notice that some ErrorMessages have no associated codes at this moment, will you plan to provide them later?

@TH3CHARLie TH3CHARLie requested a review from emmatyping August 10, 2021 17:30
@tushar-deepsource
Copy link
Contributor Author

I notice that some ErrorMessages have no associated codes at this moment, will you plan to provide them later?

Yup. Current set of PRs are pure refactors. After this is done, I'll be adding unique error codes for each and every error, like most other linters have.

@TH3CHARLie
Copy link
Collaborator

Cool, looking forward to that

@emmatyping
Copy link
Member

Could you please resolve the merge conflict?

@tushar-deepsource
Copy link
Contributor Author

Done :)

@github-actions

This comment has been minimized.

@tushar-deepsource
Copy link
Contributor Author

tushar-deepsource commented Sep 29, 2021

I have run mypy_primer locally, and there are no differences in the latest version of this branch :)

Command ran:
mypy_primer --repo https://github.com/deepsourcelabs/mypy.git -o diff --new checker-messages --old master

@tusharsadhwani
Copy link
Contributor

@ethanhs @TH3CHARLie Please take another look, anything left for me to do?

@emmatyping
Copy link
Member

The mypy primer output doesn't seem right if this is just a refactor, especially since graphql-core seems to have a bunch of type ignores become valid again...

@tusharsadhwani
Copy link
Contributor

@ethanhs I added a few more commits which I believe fixes the output, checked by running mypy-primer locally. is there some way to re trigger the action?

@tushar-deepsource
Copy link
Contributor Author

Alright, tidied up the final few bits.
@ethanhs hopefully this is the last time you'll have to trigger the workflows again

@github-actions

This comment has been minimized.

@tushar-deepsource
Copy link
Contributor Author

tushar-deepsource commented Oct 13, 2021

I was 45 commits behind master... does that explain the primer difference?
Plus I missed an explicit annotation for TRUTHY_BOOL. mypyc should run fine now.

@tusharsadhwani
Copy link
Contributor

@ethanhs PTAL?

@emmatyping emmatyping merged commit cd75e5b into python:master Nov 4, 2021
tushar-deepsource added a commit to DeepSourceCorp/mypy that referenced this pull request Jan 20, 2022
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.

4 participants