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

Reformat usage string #46

Closed
wants to merge 2 commits into from
Closed

Conversation

youssefragab99
Copy link

Related issues

If applicable: N/A

Closes/Fixes/Addresses #<45>

If not applicable: write "N/A".

Proposed changes

List out—with high level descriptions—what the commits
within this PR do:

  • Reformat usage string to contain newline between argument and description
  • Consistently formats all usage string descriptions to start with capital letter

@Pennycook Pennycook self-requested a review February 6, 2024 15:22
Copy link
Contributor

@Pennycook Pennycook left a comment

Choose a reason for hiding this comment

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

Thanks for taking a look at this. I think there are a few things that you need to address here.

Sign-off Check

Thank you for reading the contributor guide. I think you have misunderstood the intent of the sign-off check, though. Please rewrite history to remove the commit with the certificate of origin. Your remaining commit should look like this:

Add newlines to argparse help strings

Signed-off-by: Youssef Ragab <[email protected]>

Formatting

Please install and run the pre-commit hooks:

pre-commit install
pre-commit run

If you need to run on a specific file:

pre-commit run --files codebasin.py

There are some formatting changes in your patch that don't look right to me, and this should fix them.

-h Option

The -h option still has the default string, with a lower-case "show":

optional arguments:
  -h, --help            show this help message and exit

codebasin.py Outdated
Comment on lines 87 to 88
help="""
Decrease verbosity level""",
Copy link
Contributor

Choose a reason for hiding this comment

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

This inserts a newline, but doesn't seem to do so consistently.

The "Increase" and "Decrease" lines have extra indentation compared to other options:

  -r DIR, --rootdir DIR
                        Set working root directory (default .)
  -c FILE, --config FILE
                        Configuration file (default: <DIR>/config.yaml)
  -v, --verbose
                            Increase verbosity level
  -q, --quiet
                            Decrease verbosity level

Copy link
Contributor

@laserkelvin laserkelvin left a comment

Choose a reason for hiding this comment

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

Echoing @Pennycook comments, particularly regarding the guidelines laid out in the contributor's guide. Additionally, the CI check fails because commits are unsigned, so we are unable to merge it until that is done.

codebasin.py Outdated
@@ -49,8 +49,9 @@ def guess_project_name(config_path):
if __name__ == "__main__":
# Read command-line arguments
parser = argparse.ArgumentParser(
description="Code Base Investigator v" + str(version),
)
description="Code Base Investigator v" + str(version),
Copy link
Contributor

Choose a reason for hiding this comment

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

black would modify this block

codebasin.py Outdated
)
parser.add_argument(
"-v",
"--verbose",
dest="verbose",
action="count",
default=0,
help="increase verbosity level",
help="""
Copy link
Contributor

Choose a reason for hiding this comment

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

black should modify this block

codebasin.py Outdated
)
parser.add_argument(
"--batchmode",
dest="batchmode",
action="store_true",
default=False,
help="Set batch mode (additional output for bulk operation.)",
help="""
Copy link
Contributor

Choose a reason for hiding this comment

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

black should modify this block

@youssefragab99
Copy link
Author

I see the issue, thank you both for the comments @laserkelvin @Pennycook, let me see if I can find a better way of doing this.

Youssef Ragab added 2 commits February 7, 2024 00:48
Signed-off-by: Youssef Ragab <[email protected]>
…on now showing on new line with consistent indenting and formatting.

Signed-off-by: Youssef Ragab <[email protected]>'
@Pennycook
Copy link
Contributor

@youssefragab99 - Are you still working on this? Our next release is planned for the end of March, and we were hoping to have this fix included there.

@Pennycook
Copy link
Contributor

Closing as stale, and I'll open a new pull request to add the capital letters. I'll defer the formatting until the next release.
@youssefragab99, if you're still interested in working on the second part of the fix, please re-open the pull request.

@Pennycook Pennycook closed this Mar 22, 2024
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.

3 participants