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

Upgrade mypy #406

Merged
merged 2 commits into from
Jul 3, 2024
Merged

Upgrade mypy #406

merged 2 commits into from
Jul 3, 2024

Conversation

wyattscarpenter
Copy link
Contributor

This commit removes the flag (and cd step) from f53aa37 which we added to get mypy to treat namespaces correctly. This was apparently a bug in mypy, or behavior they decided to change. To get the new behavior, we must upgrade mypy. (This also allows us to remove a couple # type: ignore comment that are no longer needed.)

This commit runs changes the version of mypy and runs poetry lock; note that I did not run poetry lock --no-update, because I didn't think about it at the time (I could go do that instead if need be). It also conforms the whitespace of files in this project to the expectations of various tools and standards (namely: removing trailing whitespace as expected by git and enforcing the existence of one and only one newline at the end of a file as expected by unix and github.) It also uses https://github.com/hauntsaninja/no_implicit_optional to automatically upgrade codebase due to a change in mypy behavior. For a similar reason, it also fixes newly-found type (or otherwise) errors:

  • "Return type 'Retry' of 'new' incompatible with return type 'DatabricksRetryPolicy' in supertype 'Retry'"
  • databricks/sql/auth/retry.py:225: error: object has no attribute update [attr-defined]
  • /test_param_escaper.py:31: DeprecationWarning: invalid escape sequence ) [as it happens, I think it was also wrong for the string not to be raw, because I'm pretty sure it wants all of its backslashed single-quotes to appear literally with the backslashes, which wasn't happening until now]
  • ValueError: numpy.dtype size changed, may indicate binary incompatibility. Expected 96 from C header, got 88 from PyObject [this is like a numpy version thing, which I fixed by being stricter about numpy version]

This commit removes the flag (and cd step) from f53aa37 which we added to get mypy to treat namespaces correctly. This was apparently a bug in mypy, or behavior they decided to change. To get the new behavior, we must upgrade mypy. (This also allows us to remove a couple `# type: ignore` comment that are no longer needed.)

This commit runs changes the version of mypy and runs `poetry lock`. It also conforms the whitespace of files in this project to the expectations of various tools and standard (namely: removing trailing whitespace as expected by git and enforcing the existence of one and only one newline at the end of a file as expected by unix and github.) It also uses https://github.com/hauntsaninja/no_implicit_optional to automatically upgrade codebase due to a change in mypy behavior. For a similar reason, it also fixes a new type (or otherwise) errors:

* "Return type 'Retry' of 'new' incompatible with return type 'DatabricksRetryPolicy' in supertype 'Retry'"
* databricks/sql/auth/retry.py:225: error: object has no attribute update  [attr-defined]
* /test_param_escaper.py:31: DeprecationWarning: invalid escape sequence \) [as it happens, I think it was also wrong for the string not to be raw, because I'm pretty sure it wants all of its backslashed single-quotes to appear literally with the backslashes, which wasn't happening until now]
* ValueError: numpy.dtype size changed, may indicate binary incompatibility. Expected 96 from C header, got 88 from PyObject [this is like a numpy version thing, which I fixed by being stricter about numpy version]

---------

Signed-off-by: wyattscarpenter <[email protected]>
Copy link
Contributor

@kravets-levko kravets-levko 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 except one notice. Also, I'm curious - where all that formatting changes come from? I mean - they look okay (especially al that trailing whitespaces and blank lines), but why didn't we catch them earlier? 🤔

@wyattscarpenter
Copy link
Contributor Author

It's an interesting question. I think there are three types of formatting changes in here:

  1. Changing part of the code slightly made that longer so black put it on its own line: self-explanatory.
  2. Trailing whitespace and newlines not in src/: even though files are "supposed to" not have trailing whitespace according to git and many code-formatting tools, nothing actually enforces that or prevents this from happening; so, they just slipped in over time. (You can add git diff --check to your ci to check for trailing whitespaces (but sadly not for wrong-number-of-newlines), although this may be overkill.)
  3. Trailing whitespace and newlines in src/: this one is more confusing, because I'm pretty sure black is suppose to prevent these? But looking it over they all seem to be in comment or multi-line strings, which I guess black doesn't check (which is a reasonable choice).

I decided the most expedient way of dealing with this type error was just adding the type ignore comment back in, but with a  `[attr-defined]` specifier this time. I mean, otherwise I would have to restructure the code or figure out the proper types for a TypedDict for the dict and I don't think that's worth it at the moment.

Signed-off-by: wyattscarpenter <[email protected]>
@kravets-levko kravets-levko merged commit 93e207e into databricks:main Jul 3, 2024
11 checks passed
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