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

Update utils.py #63

Merged
merged 3 commits into from
Feb 22, 2024
Merged

Update utils.py #63

merged 3 commits into from
Feb 22, 2024

Conversation

Jk40git
Copy link
Contributor

@Jk40git Jk40git commented Feb 12, 2024

Change the _find's docstring to _load_json's

Contributor checklist


Description

Related issue

  • #ISSUE_NUMBER

Change the _find docstring to _load_json
Copy link

github-actions bot commented Feb 12, 2024

Thank you for the pull request!

The Scribe team will do our best to address your contribution as soon as we can. The following is a checklist for maintainers to make sure this process goes as well as possible. Feel free to address the points below yourself in further commits if you realize that actions are needed :)

If you're not already a member of our public Matrix community, please consider joining! We'd suggest using Element as your Matrix client, and definitely join the General and Data rooms once you're in. It'd be great to have you!

Maintainer checklist

  • The commit messages for the remote branch should be checked to make sure the contributor's email is set up correctly so that they receive credit for their contribution

    • The contributor's name and icon in remote commits should be the same as what appears in the PR
    • If there's a mismatch, the contributor needs to make sure that the email they use for GitHub matches what they have for git config user.email in their local Scribe-Data repo
  • The CHANGELOG has been updated with a description of the changes for the upcoming release and the corresponding issue (if necessary)

@andrewtavis andrewtavis mentioned this pull request Feb 12, 2024
@andrewtavis andrewtavis self-requested a review February 12, 2024 09:24
@andrewtavis
Copy link
Member

andrewtavis commented Feb 12, 2024

I see there was some confusion here, @Jk40git. Big thing is that we don't want the docstring to "be the docstring of the other one", but we want them to have the same style. For find we currently have:

    """
    Each 'language', (english, german,..., etc) is a dictionary of key/value pairs:

        entry = {
            "language": "english",
            "iso": "en",
            "qid": "Q1860",
            "remove-words": [...],
            "ignore-words": [...]
        }

    Given a key/value pair, the 'source', and the 'target' key, get the 'target' value.

    Args:
        source_value (str): e.g. 'english'.
        source_key (str): e.g. 'language'.
        target_key (str): e.g. 'iso'.
        error_msg (str): for when a value cannot be found.

    Raises:
        ValueError: when a source_value is not supported.

    Returns:
        The 'target' value given the passed arguments.
    """

What this should be is:

    """
    Each 'language', (english, german,..., etc) is a dictionary of key/value pairs:

        entry = {
            "language": "english",
            "iso": "en",
            "qid": "Q1860",
            "remove-words": [...],
            "ignore-words": [...]
        }

    Given a key/value pair, the 'source', and the 'target' key, get the 'target' value.

    Parameters
    ----------
        source_value : str
            The source value to find equivalents for (e.g. 'english').
        source_key : str
            The source key to reference (e.g. 'language').
        target_key : str
            The key to target (e.g. 'iso').
        error_msg : str
            The message displayed when a value cannot be found.

    Raises
    ------
        ValueError : when a source_value is not supported.

    Returns
    -------
        The 'target' value given the passed arguments.
    """

The big thing is that everywhere that we have Args: in a docstring we want:

Parameters
----------
    {LIST_OF_THE_ARGS}

And everywhere we have Returns: we want:

Returns
-------
    {LIST_OF_RETURN_VALUES}

Can you convert the docstring of _find over to what I have above, and then I can send along a list of functions that we also need to do other conversions for?

Changes to docstring of  _find
@Jk40git
Copy link
Contributor Author

Jk40git commented Feb 12, 2024

@andrewtavis Just made an edit #63

@andrewtavis
Copy link
Member

Nice, yes, this is looking good @Jk40git! Here are the other docstrings that need edits:

  • In checkquery.py basically all for function docstrings need to be edited
  • In utils.py we need _find as you already did and then check_command_line_args and check_and_return_command_line_args

Want to send along another commit with all of these changes, and then we can close this up? In cases where there's a section that's not Args: or Returns:, let's just keep it in order and put dashes under it. So for example Raises: we can also do:

Raises
------

as in the example above :) Let me know if there are any questions on this!

@Jk40git
Copy link
Contributor Author

Jk40git commented Feb 12, 2024

Nice, yes, this is looking good @Jk40git! Here are the other docstrings that need edits:

  • In checkquery.py basically all for function docstrings need to be edited
  • In utils.py we need _find as you already did and then check_command_line_args and check_and_return_command_line_args

Want to send along another commit with all of these changes, and then we can close this up? In cases where there's a section that's not Args: or Returns:, let's just keep it in order and put dashes under it. So for example Raises: we can also do:

Raises
------

as in the example above :) Let me know if there are any questions on this!

Do you want me to change the check_command_line_args and check_and_return_command_line_args with the same docstring from _find?

Copy link
Member

@andrewtavis andrewtavis left a comment

Choose a reason for hiding this comment

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

Minor changes sent along in the most recent commit finishes this up, @Jk40git. Thanks! Will move to the others now :)

@andrewtavis
Copy link
Member

Don't worry about the Mac builds, by the way. This is known in #61 :)

@andrewtavis andrewtavis merged commit 9a87f25 into scribe-org:main Feb 22, 2024
3 of 6 checks passed
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.

2 participants