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

Replace typing-extensions Literal with the type of the Literal #677

Merged
merged 6 commits into from
Oct 12, 2021

Conversation

pietermarsman
Copy link
Member

@pietermarsman pietermarsman commented Oct 11, 2021

Pull request

Fixes #676

  • Removes typing-extensions Literal and replaces it with the type of the Literal.
  • Changes return type of open_filename contextmanager __exit__ method from False to None.
  • Split tox.ini environments into 4 parts. One for mypy, flake8, nosetests and sphinx.This should fail for the first commit.

How Has This Been Tested?

Made the cicd fail when changing tox.ini. Then fixed it by removing the typing-extensions.

Checklist

  • I have added tests that prove my fix is effective or that my feature
    works
  • I have added docstrings to newly created methods and classes
  • I have optimized the code at least one time after creating the initial
    version
  • I have updated the README.md or I am verified that this
    is not necessary
  • I have updated the readthedocs documentation or I
    verified that this is not necessary
  • I have added a consice human-readable description of the change to
    CHANGELOG.md

@pietermarsman pietermarsman changed the title Add typing-extensions as a runtime dependency and make sure that cicd catches missing runtime dependencies Replace typing-extensions Literal with the type of the Literal Oct 11, 2021
@0xabu
Copy link
Contributor

0xabu commented Oct 11, 2021

@pietermarsman sorry for the mess. This PR looks way more complex than I anticipated. CI issues aside, is there a reason we can't just add typing-extensions to our runtime dependencies in setup.py?

…e own environment.

Improves isolation. Dependencies of one package won't influence the next.

This should fail for the current setup with typing-extensions.
@pietermarsman pietermarsman force-pushed the fix-uninstalled-typing-extensions branch from fb9b83e to 35221fb Compare October 11, 2021 20:41
@0xabu
Copy link
Contributor

0xabu commented Oct 11, 2021

is there a reason we can't just add typing-extensions to our runtime dependencies in setup.py?

See #678 for what I mean.

@pietermarsman
Copy link
Member Author

Yep, I understand.

Not sure yet. I like to keep dependencies minimal but this one seems ok. Let's discuss in gitter.

I wanted to improve the cicd in any case such that it will raise errors in the future.

@0xabu
Copy link
Contributor

0xabu commented Oct 11, 2021

Agreed on the CI improvements.

As for the dependency -- no big deal either way. There's only one non-trivial use of Literal in some questionable code in pdf2txt, and I hope to send you a bigger PR that cleans it up anyway.

(Hint: #657 fixes a related open issue there.)

@htInEdin
Copy link

For the actual fix to psparser and pdf2txt, I prefer e.g.
Union[float, typing.Literal("disabled") if typing.hasattr('Literal') else str]
because it's future-proof.

@pietermarsman
Copy link
Member Author

@0xabu I'm not going to include the typing-extensions for now. The __exit__ typing is fixed by returning None (returning False makes only sense if returning True is also an option, which wasn't). And then there is just one use case left for the typing-extensions package.

Don't hesitate to add it in the future in another PR, if you have additional use cases for it. I just think that having one usage of it is not enough.

@pietermarsman pietermarsman merged commit 104883d into develop Oct 12, 2021
@pietermarsman pietermarsman deleted the fix-uninstalled-typing-extensions branch October 12, 2021 18:23
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.

Add typing-extensions as a runtime dependency
3 participants