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 support for YAML data files in load_data [RHELDST-4904] #21

Merged
merged 2 commits into from
Feb 3, 2021

Conversation

crungehottman
Copy link
Member

The load_data method will accept both YAML and JSON files, but will
load the data.yaml file by default.

This change enables cdn-definitions-private to cease maintaining
two sets of data, in both YAML and JSON forms -- only data.yaml
files will be provided and maintained. Those who wish to use
the cdn-definitions-private data will be able to do so without
a manual conversion of the YAML data file into a JSON file.

Copy link
Member

@rohanpm rohanpm left a comment

Choose a reason for hiding this comment

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

Only complaint is about adding the pathlib dep.

requirements.txt Outdated Show resolved Hide resolved
The load_data method will accept both YAML and JSON files, but will
load the data.yaml file by default.

This change enables cdn-definitions-private to cease maintaining
two sets of data, in both YAML and JSON forms -- only data.yaml
files will be provided and maintained. Those who wish to use
the cdn-definitions-private data will be able to do so without
a manual conversion of the YAML data file into a JSON file.
@@ -43,15 +49,15 @@ def __init__(self, **kwargs):

def rhui_aliases():
"""Returns:
list[:class:`~PathAlias`]
A list of aliases relating to RHUI paths.
list[:class:`~PathAlias`]
Copy link
Member

Choose a reason for hiding this comment

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

Have you verified that this displays correctly on the rendered docs?

The indent in general is meaningful. This commit is changing the string from:

Returns:
    list[:class:`~PathAlias`]
        A list of aliases relating to RHUI paths.

to:

Returns:
list[:class:`~PathAlias`]
    A list of aliases relating to RHUI paths.

Doc strings are processed via this tool: https://www.sphinx-doc.org/en/master/usage/extensions/napoleon.html

And in the examples there you can see that the docs for return value are always indented under "Returns", so I have a doubt whether this change is correct.
I'm not sure if the doc string parser is still going to process this the same way as it did before...

Copy link
Member

Choose a reason for hiding this comment

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

Is it black tool reformatting this?

Copy link
Member

Choose a reason for hiding this comment

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

I see now there's a separate commit in the PR saying that it's coming from black.

I checked this locally and I'm a bit confused actually, as both before and after this change sphinx generates warnings:

/home/rmcgover/src/cdn-definitions/.tox/docs/lib/python3.8/site-packages/cdn_definitions/_impl/__init__.py:docstring of cdn_definitions._impl.rhui_aliases:3: WARNING: Unexpected indentation.
/home/rmcgover/src/cdn-definitions/.tox/docs/lib/python3.8/site-packages/cdn_definitions/_impl/__init__.py:docstring of cdn_definitions._impl.origin_aliases:3: WARNING: Unexpected indentation.

So maybe it was not correct either before or after? However, the rendered docs seem OK to me, so no objections to this.

@rohanpm rohanpm self-requested a review February 3, 2021 03:47
@rohanpm rohanpm merged commit cf554b8 into release-engineering:master Feb 3, 2021
@crungehottman crungehottman deleted the add-yaml-support branch February 3, 2021 16:32
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.

None yet

2 participants