Conversation
This PR adds running codespell to the scripts/test script, which is run by CI. This will fail PRs that have spelling mistakes. A currently empty ignore file is put into place in case codespell fails on any intentional spellings. Fixes #200
Codecov Report
@@ Coverage Diff @@
## develop #206 +/- ##
========================================
Coverage 92.32% 92.32%
========================================
Files 28 28
Lines 3349 3349
========================================
Hits 3092 3092
Misses 257 257 Continue to review full report at Codecov.
|
schwehr
left a comment
schwehr
left a comment
There was a problem hiding this comment.
I can't figure out how to comment on an empty file...
For .codespellignore, I think you'll need:
ACI
ALOS
NED
UE
|
|
||
| # Code spelling | ||
| codespell -I .codespellignore -f \ | ||
| pystac/*.py pystac/**/*.py \ |
There was a problem hiding this comment.
Why not just do dirs? It looks like you are leaving out the test data. There are only a few things needed for the codespellignore for the test data.
codespell -I .codespellignore -f docs pystac scripts tests *.py *.mdThere was a problem hiding this comment.
Trying to just specify directories was including things like docs/_build/html/_static/jquery.js and .ipynb_checkpoints and __pycache__ files for me, and throwing a lot of warnings about binary files.
Does that command cleanly for you?
There was a problem hiding this comment.
codespell on my machine doesn't seem to pay attention to .codespellignore, but if you add --skip _build,__pycache__, that seems to work for me minus the two examples files.
codespell --skip _build,__pycache__ -I .codespellignore -f docs pystac scripts tests *.py *.md
tests/data-files/examples/gee-0.6.2/catalog.json:18: ACI ==> ACPI
tests/data-files/examples/gee-0.6.2/catalog.json:438: ALOS ==> ALSO
tests/data-files/examples/gee-0.6.2/catalog.json:443: ALOS ==> ALSO
tests/data-files/examples/gee-0.6.2/catalog.json:448: ALOS ==> ALSO
tests/data-files/examples/gee-0.6.2/catalog.json:453: ALOS ==> ALSO
tests/data-files/examples/gee-0.6.2/catalog.json:2218: NED ==> NEED
tests/data-files/examples/sentinel-0.6.0/sentinel-2-l1c/9/V/catalog.json:1: UE ==> USE, DUE
There was a problem hiding this comment.
Skipping the directories mentioned worked for avoiding those files, but it ended up picking up my venv files. Also I'll note that I don't think spell checking the data-files is necessary; misspellings from other folk's STAC is harmless and wont' affect tests. I think approach this as being explicit about what we want to include rather than doing everything but specific excludes will make for a better experience.
There was a problem hiding this comment.
Really either way is fine and gets the project better coverage than now. Personally, I'd check the data files too. Errors cause friction in unexpected ways for in different circumstances and tend to propagate via copy paste, but these are unlikely to cause too much trouble.
There was a problem hiding this comment.
Makes sense. I think keeping it to code and documentation works for now, and we can expand it if we see we're running into situations where we wish we were spell checking stacs that are brought in.
Can you move your review to Approved if this is resolved?
3 participants
This PR adds running codespell to the scripts/test script, which is run by CI. This will fail PRs that have spelling mistakes. A currently empty ignore file is put into place in case codespell fails on any intentional spellings.
Fixes #200