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

no_implicit_optional A codemod to make your implicit optional type hints PEP-484 compliant #116

Merged
merged 2 commits into from
Apr 10, 2024

Conversation

cleder
Copy link
Contributor

@cleder cleder commented Apr 10, 2024

workerB

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

Nice! Thanks!

@sobolevn sobolevn merged commit dd5e16d into typeddjango:master Apr 10, 2024
1 check passed
@orsinium
Copy link
Collaborator

We already have auto-optional. I wonder if we should pick one.

@cleder
Copy link
Contributor Author

cleder commented Apr 11, 2024

Good, point, they seem to do the same thing, auto-optional also has a pre-commit plugin.

@cleder
Copy link
Contributor Author

cleder commented Apr 11, 2024

Looking through the code and tests, I tend to lean towards auto-optional, but I may be wrong. (Disclaimer I did not test either yet, I don't have code atm where this would be a problem)

@orsinium
Copy link
Collaborator

I tried both on a fairly big codebase and no_implicit_optional seemed to fare better. Both tools use libcst under the hood, and in both cases it fails with SyntaxError on some ruff-formatted with statements. However, auto-optional stopped formatting files at the first error while no_implicit_optional skipped formatting only for the failing files and says that it formatted the rest. Plus, no_implicit_optional supports the new union syntax

However, in both cases, I don't see any files actually modified, even if the tool says it did. IDK why. Some kind of dry run? 🤔

@orsinium
Copy link
Collaborator

Ah, no, I see, I already fixed implicit optionals in that project, that's why 🙃

@orsinium
Copy link
Collaborator

Lets leave both on the list 👍🏿

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants