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

Would you be interested in a PR adding type hints? #362

Closed
Recursing opened this issue Jan 22, 2020 · 14 comments
Closed

Would you be interested in a PR adding type hints? #362

Recursing opened this issue Jan 22, 2020 · 14 comments

Comments

@Recursing
Copy link
Contributor

I think type hints would help users and new contributors

@pietermarsman
Copy link
Member

Yes, that's interesting! As long as it does not break anything; I am not sure if type hints are supported by Python3.4.

Some additional thoughts:

  • you can start by adding types to the tools (e.g. pdf2txt.py and dumppdf.py), the high_level.py functions and the most used classes (e.g. PDFDocument, TextConverter, PDFPageAggregator, LAParams, PDFResourceManager, PDFPageInterpreter and PDFPage).
  • mypy is a static type checker that could enforce the correct usage of types
  • be sure to read pep484

@Recursing
Copy link
Contributor Author

Python 3.4 does not support inline type annotations, so the valid syntax would be annoyingly verbose (and non-standard). But why do you guys even support it? I think it's not that common anymore

@pietermarsman
Copy link
Member

I also think that very few people use that. It is much easier to upgrade from python 3.4 than it was from e.g. 2.7.

Let me check with other contributors to see what they think.

@jstockwin jstockwin added this to new in pdfminer.six via automation Jul 9, 2020
@jstockwin jstockwin moved this from new to needs solution in pdfminer.six Jul 9, 2020
@jstockwin
Copy link
Member

@pietermarsman Did you get any response from other contributors? In my view I think it's fine to drop python 3.4 support, and type hints would be nice.

The only drawback is it might be a slight blocker for other people contributing if they don't know how to use them.

@Recursing I appreciate it's been a long time, but we're currently trying to sort through all the open issues. Is this something you'd still be interested in helping with?

@Recursing
Copy link
Contributor Author

The only drawback is it might be a slight blocker for other people contributing if they don't know how to use them.

They don't need to be mandatory. I think they would definitely help more than harm

Is this something you'd still be interested in helping with?

Sure

@pietermarsman
Copy link
Member

@pietermarsman Did you get any response from other contributors? In my view I think it's fine to drop python 3.4 support, and type hints would be nice.

Not that I remember.

I like type annotations so I am pro :)

@jstockwin
Copy link
Member

I also like them so I vote yes

@pietermarsman
Copy link
Member

When we add this, we should also add mypy to the travis-ci test run. I just ran it locally, and there are only 8 issues. This surprised me a bit, but maybe its because we dont have type annotations yet. With mypy the type annotations add more value because we detect wrong usage of types earlier.

@jstockwin
Copy link
Member

Agreed. Potentially also worth adding pytype as the two tools check different things

@pietermarsman
Copy link
Member

I also wanted to add mypy to a work-related project, but after you suggested pytype I did a little research into that. And now I actually prefer pytype over mypy because of two benefits:

  • pytype infers types from initialization methods, e.g. if you use str it infers the variable is a str. Mypy does not infer types and only relies on type annotations so much less coverage.
  • It does type inference for every line, where mypy allows a single variable to have a single type during its entire lifetime. The latter might sound more strict and understandable, but it does not allow to map 'str' over a list or do multiple operations to the same variable.

See this presentation (by creator of pytype)

@jstockwin
Copy link
Member

Yeah, I agree it's good. However, there are some things that mypy catches that pytype won't, so using both is a valid option.

@pietermarsman
Copy link
Member

Lets leave it up to the one who implements it then :p I've moved this to accepted.

@djbrown
Copy link

djbrown commented Mar 14, 2021

Wouldn't it be best to add the stubs in https://github.com/python/typeshed?
Then the type hints would be completely optional.
I'm currently looking into this with local stubs.

@0xabu 0xabu mentioned this issue Aug 20, 2021
@pietermarsman
Copy link
Member

Closed by #661

pdfminer.six automation moved this from accepted to done Oct 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
pdfminer.six
  
done
Development

No branches or pull requests

4 participants