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

Add test for package readme syntax errors #492

Merged
merged 12 commits into from
Mar 28, 2020
1 change: 1 addition & 0 deletions dev-requirements.txt
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,4 @@ sphinx-rtd-theme~=0.4
sphinx-autodoc-typehints~=1.10.2
pytest!=5.2.3
pytest-cov>=2.8
readme-renderer==24.0
Oberon00 marked this conversation as resolved.
Show resolved Hide resolved
4 changes: 2 additions & 2 deletions ext/opentelemetry-ext-otcollector/README.rst
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ OpenTelemetry Collector Exporter
.. |pypi| image:: https://badge.fury.io/py/opentelemetry-ext-otcollector.svg
:target: https://pypi.org/project/opentelemetry-ext-otcollector/

This library allows to export data to `OpenTelemetry Collector <https://github.com/open-telemetry/opentelemetry-collector/>`_ , currently using OpenCensus receiver in Collector side.
This library allows to export data to `OpenTelemetry Collector`_ , currently using OpenCensus receiver in Collector side.

Installation
------------
Expand Down Expand Up @@ -95,4 +95,4 @@ References
----------

* `OpenTelemetry Collector <https://github.com/open-telemetry/opentelemetry-collector/>`_
* `OpenTelemetry Project <https://opentelemetry.io/>`_
* `OpenTelemetry <https://opentelemetry.io/>`_
6 changes: 6 additions & 0 deletions scripts/eachdist.py
Original file line number Diff line number Diff line change
Expand Up @@ -482,6 +482,12 @@ def lint_args(args):
args, ("exec", "pylint {}", "--all", "--mode", "lintroots")
)
)
execute_args(
parse_subargs(
args,
("exec", "python tests/check_for_valid_readme.py {}", "--all",),
)
)


def test_args(args):
Expand Down
53 changes: 53 additions & 0 deletions tests/check_for_valid_readme.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
"""Test script to check README.rst files for syntax errors."""
import argparse
import sys
from pathlib import Path

import readme_renderer.rst


def is_valid_rst(path):
"""Checks if RST can be rendered on PyPI."""
with open(path) as f:
markup = f.read()
if readme_renderer.rst.render(markup) is None:
return False
return True
ffe4 marked this conversation as resolved.
Show resolved Hide resolved


def parse_args():
parser = argparse.ArgumentParser(
description="Checks README.rst file in path for syntax errors."
)
parser.add_argument(
"paths", nargs="+", help="paths containing a README.rst to test"
)
parser.add_argument("-v", "--verbose", action="store_true")
return parser.parse_args()


if __name__ == "__main__":
args = parse_args()
ffe4 marked this conversation as resolved.
Show resolved Hide resolved

error = False
all_readmes_found = True

for path in [Path(path) for path in args.paths]:
ffe4 marked this conversation as resolved.
Show resolved Hide resolved
readme = path / "README.rst"
if not readme.exists():
Copy link
Member

Choose a reason for hiding this comment

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

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.

all_readmes_found = False
print("✗ No README.rst in", str(path))
ffe4 marked this conversation as resolved.
Show resolved Hide resolved
continue
if not is_valid_rst(readme):
error = True
print("✗ RST syntax errors in", readme)
continue
if args.verbose:
print("✓", readme)

if error:
sys.exit(1)
if all_readmes_found:
print("All clear.")
else:
print("No errors found but not all packages have a README.rst")
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exclusions do not seem to be supported, so I added a README to the testutil package for now.

1 change: 1 addition & 0 deletions tox.ini
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,7 @@ deps =
isort
black
psutil
readme_renderer

commands_pre =
python scripts/eachdist.py install --editable
Expand Down