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

build improvements #109

Merged
merged 12 commits into from
May 3, 2022
Merged

Conversation

philvarner
Copy link
Contributor

@philvarner philvarner commented Mar 1, 2022

Numerous build improvements:

  • add 3.9 and 3.10 to the text matrix
  • update black, isort, and flake8 versions
  • add instructions to install pre-commit hooks and how to run tox
  • remove all unused imports
  • change line length to 120 b/c there are a lot that are longer than 90
  • fixup warnings from flake8
  • not sure why numpy was a dep for running tox?
  • add config for running mypy, but leave commented since there are a number of problems to fix

@philvarner philvarner marked this pull request as draft March 1, 2022 20:37
@philvarner philvarner marked this pull request as ready for review March 1, 2022 20:42
tox.ini Outdated
exclude = .git,__pycache__,docs/source/conf.py,old,build,dist
max-line-length = 90
exclude = .git,__pycache__,docs/source/conf.py,old,build,dist,.venv,.tox,.idea
max-line-length = 120
Copy link
Contributor

Choose a reason for hiding this comment

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

Regardless of the value here, black defaults to 88, so they suggest setting the flake8 config to 88: https://black.readthedocs.io/en/stable/the_black_code_style/current_style.html#line-length

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was wondering about that -- black wasn't giving an error but flake8 was on those lines. I've updated the flake8 config per the black documentation.

@codecov-commenter
Copy link

codecov-commenter commented Mar 1, 2022

Codecov Report

Merging #109 (7e62410) into master (88bac55) will increase coverage by 0.55%.
The diff coverage is 98.92%.

@@            Coverage Diff             @@
##           master     #109      +/-   ##
==========================================
+ Coverage   96.27%   96.82%   +0.55%     
==========================================
  Files          20       20              
  Lines         483      473      -10     
==========================================
- Hits          465      458       -7     
+ Misses         18       15       -3     
Flag Coverage Δ
unittests 96.82% <98.92%> (+0.55%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
stac_pydantic/__init__.py 100.00% <ø> (ø)
stac_pydantic/api/__init__.py 100.00% <ø> (ø)
stac_pydantic/api/search.py 96.25% <94.11%> (+0.04%) ⬆️
stac_pydantic/api/collections.py 90.90% <100.00%> (ø)
stac_pydantic/api/extensions/context.py 100.00% <100.00%> (ø)
stac_pydantic/api/extensions/fields.py 91.30% <100.00%> (ø)
stac_pydantic/api/extensions/sort.py 100.00% <100.00%> (ø)
stac_pydantic/api/landing.py 100.00% <100.00%> (ø)
stac_pydantic/api/utils/link_factory.py 100.00% <100.00%> (ø)
stac_pydantic/catalog.py 100.00% <100.00%> (ø)
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 88bac55...7e62410. Read the comment docs.

Comment on lines 21 to 26
# - repo: https://github.com/pre-commit/mirrors-mypy
# rev: v0.931
# hooks:
# - id: mypy
# exclude: ^tests/
# args: [--strict]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this meant to be commented out?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Feel free to disregard: just saw that this has an answer in the PR description

@moradology
Copy link
Collaborator

For some reason pre-commit gave me some more type issues than my local mypy run... I should have this polished up later tonight or tomorrow morning

Copy link
Collaborator

@geospatial-jeff geospatial-jeff left a comment

Choose a reason for hiding this comment

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

The build changes look good to me, although there are other breaking changes sprinkled through the PR (like removing of Link.__iter__) and several others which means we will have to release as a new major version.

I think I would like to separate the build improvements from the functionality change to isolate the breaking code changes in a different PR.

@moradology
Copy link
Collaborator

moradology commented May 3, 2022

Yeah, I think you make a totally valid point. A bit of context on the breaking changes: the shift from Link.__iter__ to Link.link_iterator is explicitly to avoid breaking a substitution rule which mypy enforces. In general, where breaking changes happened, it was in attempting to fix the numerous mypy errors. Is the preferred approach to remove such changes and sprinkle in # type: ignores?

@moradology
Copy link
Collaborator

After further discussion, it looks like the most sensible move might be to accept this PR after cutting a minor release. This will avoid the complexities of getting this PR's non-breaking bits in without the breaking portions.

@moradology moradology merged commit 86f7303 into stac-utils:master May 3, 2022
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.

5 participants