Add test for package readme syntax errors #492
Conversation
Fixes invalid references in README and fixes errors in 26bddb7
tests/check_for_valid_readme.py
Outdated
| if all_readmes_found: | ||
| print("All clear.") | ||
| else: | ||
| print("No errors found but not all packages have a README.rst") |
There was a problem hiding this comment.
Any reason not to treat this as an error and exit as above? I would assume we'd want a README.rst for all packages.
There was a problem hiding this comment.
The package does not technically need to have a README.rst. The file could be named differently, it could be an markdown or plain text file, or the description could be embedded inline, instead of in a separate file. testutil does not have a README and seems to be marked for deletion (#374 5.), so I felt this behavior might be desirable.
If someone adds a README.rst it will get checked, and if they do not include a README.rst we just assume that the package is still WIP and/or will not be uploaded to PyPI. If there is another README file the warning basically indicates departure from implicit convention.
There was a problem hiding this comment.
We can just force everyone to put the readme in README.rst. If we want to allow README.md, then this tool will need to support it too.
If we want to exclude packages, I think we can add the exclusion list to eachdist.ini (but I'm not sure if it has support for configured exclusions yet).
There was a problem hiding this comment.
Exclusions do not seem to be supported, so I added a README to the testutil package for now.
Codecov Report
@@ Coverage Diff @@
## master #492 +/- ##
=======================================
Coverage 89.47% 89.47%
=======================================
Files 43 43
Lines 2195 2195
Branches 247 247
=======================================
Hits 1964 1964
Misses 159 159
Partials 72 72 Continue to review full report at Codecov.
|
tests/check_for_valid_readme.py
Outdated
|
|
||
| for path in [Path(path) for path in args.paths]: | ||
| readme = path / "README.rst" | ||
| if not readme.exists(): |
There was a problem hiding this comment.
I would move this check under the if not is_valid_rst (or in a try/except around it). We should first try to parse the readme, and if parsing failed we check to see if a nonexistent file was the reason.
tests/check_for_valid_readme.py
Outdated
| if all_readmes_found: | ||
| print("All clear.") | ||
| else: | ||
| print("No errors found but not all packages have a README.rst") |
There was a problem hiding this comment.
We can just force everyone to put the readme in README.rst. If we want to allow README.md, then this tool will need to support it too.
If we want to exclude packages, I think we can add the exclusion list to eachdist.ini (but I'm not sure if it has support for configured exclusions yet).
Commit via GitHub web interface. Co-Authored-By: Christian Neumüller <christian+github@neumueller.me>
Also adds README.rst for testutil package to pass new test.
There was a problem hiding this comment.
@ffe4 thanks for the change! And it looks good. Only change is to move the script itself to ./scripts, to follow the standard convention of all of our helper scripts.
See #445
I’ve gone ahead and wrote a simple test script that assumes a
README.rstis and should be present. Not sure what output and exit behavior would be desirable in that case. I exit normally and print a warning if theREADME.rstis missing.sys.exit(1)if there is a syntax error. Example output of is below.There is apparently already an error in one of the readme files which I'll look at tomorrow(fixed in 9ddbfd8). Would something like this be sufficient?