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

Homogenize annotation and typing syntax #53

Closed
rhugonnet opened this issue Mar 14, 2021 · 3 comments · Fixed by #306
Closed

Homogenize annotation and typing syntax #53

rhugonnet opened this issue Mar 14, 2021 · 3 comments · Fixed by #306
Labels
documentation Improvements or additions to documentation enhancement Feature improvement or request

Comments

@rhugonnet
Copy link
Member

Right now it's still a mix. I'm up for homogenizing for in-function annotations everywhere (Erik's good at it, but I'm slowly learning ;D)

@rhugonnet rhugonnet added the documentation Improvements or additions to documentation label Mar 14, 2021
@erikmannerfelt
Copy link
Contributor

I support this!

The linters I use are: mypy (git version), prospector (pylint + pep8 + McCabe complexity) and pydocstyle. The formatters I use are: isort and autopep8.

The latest release of mypy (December 2020) strangely doesn't like the list[subtype] syntax, even after including from __future__ import annotations. I therefore use the latest source version instead, which has included a fix (pip install git+https://github.com/python/mypy.git).

Linters

mypy is a static type helper, for example giving an error if you write def func(value: int = 1.1) -> float (1.1 is not an int), or try to subsequently use the function in an incorrect manner: "hello" + func(3) (str + float doesn't work).

pydocstyle makes sure docstrings are more or less correctly formatted. It doesn't enforce e.g. sphinx formatting, so maybe there's another tool to add here?

prospector is a convenience linter that implements three linters in one:

pylint is a general-purpose linter for common issues. For example, it enforces the correct use of "snake_case" and "CamelCase" naming conventions.

pep8 warns you about line lengths, weird whitespace, wrong indentation, correct line spacing between functions, etc.

McCabe complexity makes sure that functions and classes are not overly complex, e.g. if a function has more than X defined variables, maybe it should be split in two. I am particularly bad at this..

Formatters

isort sorts imports alphabetically and divides them in three categories: "builtin imports", "third party imports", "local imports" (if the import is a directory in your cwd for example).

autopep8 helps with automatically conforming your code to the pep8 standard.

I've set the autopep8 and pylint maximum column width to 120 lines instead of 80 (read Linus Torvalds' take on this!)

Should we agree on a combination of linters and formatters? I know some repos have commit/push/merge hooks that format the code. Since we are only ~three active developers right now, maybe this is overly complex at the time being, but we could consider something similar in the future.

@adehecq
Copy link
Member

adehecq commented Mar 15, 2021

Maybe the issue should be renamed "Write a contribution guideline" with all this info ;-)
https://docs.github.com/en/github/building-a-strong-community/setting-guidelines-for-repository-contributors

@rhugonnet
Copy link
Member Author

This is amazing @erikmannerfelt ! 😳
Learning a great lot thanks to you.

Should we agree on a combination of linters and formatters? I know some repos have commit/push/merge hooks that format
the code. Since we are only ~three active developers right now, maybe this is overly complex at the time being, but we could
consider something similar in the future.

I agree that this is still early. It would be great for us three to try to abide by those standards, which are really useful even for us when coding (in PyCharm, I simply point my mouse at a function I'm using and I get all input/output types in detail, it's truly amazing), and to make everything homogeneous.
#56 will be super helpful, even for the next GlacioHack.

I guess we should aim at switching the annotations of geoutils too, for consistency? It's good practice too ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement Feature improvement or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants