Add mypy + pytest global config + small fixes.#259
Add mypy + pytest global config + small fixes.#259benjaminrwilson wants to merge 20 commits intomasterfrom
Conversation
| [tool.mypy] | ||
| ignore_missing_imports = true | ||
| strict = true |
There was a problem hiding this comment.
mypy settings are supported in pyproject.toml now.
There was a problem hiding this comment.
is there a specific version of mypy that we need to enforce, to be able to use this functionality?
There was a problem hiding this comment.
Do we use mypy anywhere other than: https://github.com/argoai/argoverse-api/blob/ceaee0f0e09281fc36f58900388a0d00fd6841f9/tox.ini?
| [tool.pytest.ini_options] | ||
| addopts = "--cov argoverse --cov-append --cov-branch --cov-report=term-missing" |
There was a problem hiding this comment.
Add pytest settings into pyproject.toml.
argoverse/__init__.py
Outdated
| @@ -0,0 +1 @@ | |||
| VERSION = "1.1.0" | |||
There was a problem hiding this comment.
Used in setup.cfg.
There was a problem hiding this comment.
is the version required to be placed here? This probably wouldnt be a place I would look for the version number, so do we need to write it in 2 places now?
There was a problem hiding this comment.
This is the first method listed here: https://packaging.python.org/guides/single-sourcing-package-version/#single-sourcing-the-package-version. Although, they use __version__ instead of VERSION.
There was a problem hiding this comment.
Updated to use __version__.
| [tool.black] | ||
| line-length = 120 | ||
|
|
There was a problem hiding this comment.
Sorted alphabetically.
setup.cfg
Outdated
| @@ -1,2 +1,59 @@ | |||
| [metadata] | |||
There was a problem hiding this comment.
Static declaration of metadata previously listed in setup.py.
| pytest tests | ||
| flake8 argoverse | ||
| mypy argoverse | ||
|
|
There was a problem hiding this comment.
These flags reside in either setup.cfg or pyproject.toml now.
|
Thanks for the PR, Ben. Could you add to the PR description a few more details about the advantages of making this change? |
|
Could you add the version numbers as well, also? Do we need a specific version of |
setup.cfg
Outdated
| version = attr: argoverse.VERSION | ||
|
|
||
| [options] | ||
| zip_safe = False |
There was a problem hiding this comment.
why do we want zip_safe = False?
There was a problem hiding this comment.
This was just a default option listed here: https://setuptools.readthedocs.io/en/latest/userguide/declarative_config.html?highlight=zip_safe. I don't have a strong opinion on it. setuptools will automatically try to determine the setting if we remove it, which might be useful.
| pytest tests --cov argoverse --cov-append --cov-branch --cov-report=term-missing | ||
| flake8 --max-line-length 120 --ignore E203,E704,E711,E722,E741,W291,W293,W391,W503,F821,F401,F811,F841,P101,G004,G002,I201,I100,I101 --enable-extensions G argoverse | ||
| mypy --ignore-missing --strict argoverse | ||
| pytest tests |
There was a problem hiding this comment.
have we verified that these options get picked up from the pyproject.toml file in practice?
There was a problem hiding this comment.
Some of these can be verified by looking at the tox run in our Github workflow. I'll verify once again after we've settled on the other changes.
Which version numbers are you referring to?
|
tagarwal-argoai
left a comment
tagarwal-argoai
left a comment
There was a problem hiding this comment.
I took a look at the PR and it looks good overall. I am not an expert with setup.cfg but I trust your and John’s review on this and you could proceed with landing this.
benjaminrwilson
changed the title
4 participants
No description provided.