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

Refactor usage of args (excluding *args) to argv #33

Merged
merged 1 commit into from
Jan 25, 2024
Merged

Refactor usage of args (excluding *args) to argv #33

merged 1 commit into from
Jan 25, 2024

Conversation

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.

Thank you again for working on this.

I didn't appreciate that we had a few different conventions for args here -- thank you for being thorough and looking for them all! I think there are a few cases here where renaming things to argv isn't appropriate; I've convinced myself that it only makes sense to use argv as a name when the variable is a list of strings representing a command (like one would get from sys.argv).

I've gone through the changes and highlighted cases where I think we should revert to args in keeping with the logic above. @laserkelvin, could you also take a quick look to see if you agree that this convention makes sense?

Code changes aside, it would be great if you could also:

  • Rebase to remove the first two (unused) commits
  • Amend the commit to include a sign-off (with git commit -s)

I suspect it would be easiest to do both of these things at the same time, by squashing everything into one (signed) commit. You can do that without opening a new pull request, by making your changes locally and then force-pushing to the same branch.

codebasin.py Outdated Show resolved Hide resolved
codebasin/preprocessor.py Outdated Show resolved Hide resolved
codebasin/preprocessor.py Outdated Show resolved Hide resolved
etc/coverage.py Outdated Show resolved Hide resolved
etc/preprocess.py Outdated Show resolved Hide resolved
tests/macro_expansion/test_macro_expansion.py Outdated Show resolved Hide resolved
tests/parsers/test_directive_parser.py Outdated Show resolved Hide resolved
@itsjayway itsjayway closed this Jan 15, 2024
@itsjayway itsjayway reopened this Jan 15, 2024
@itsjayway
Copy link
Contributor Author

thank you for your feedback. I've pushed a commit that keeps the argv changes where they relate to commands.

I'm not sure how I erased my commit history, but hopefully this change is appropriate

@Pennycook
Copy link
Contributor

thank you for your feedback. I've pushed a commit that keeps the argv changes where they relate to commands.

Thanks, @itsjayway. The changes look good now.

I'm not sure how I erased my commit history, but hopefully this change is appropriate

Don't worry about erasing the commit history -- we wanted that to happen, to get rid of the revert commit and other work-in-progress commits.

But I'm afraid I'm going to need to ask you to rebase and force push one more time because of our contributing guidelines. Specifically:

To give you an idea of what we're looking for, something like the below would be appropriate:

Refactor usage of args to argv

Applies only to commands accepting a list of string arguments.

Signed-off-by: Your Name <[email protected]>

Applies only to commands accepting a list of string arguments.

Signed-off-by: Jibran Absarulislam <[email protected]>
@Pennycook Pennycook merged commit 2ab2396 into intel:main Jan 25, 2024
1 check passed
@Pennycook
Copy link
Contributor

Thank you for contributing! 🎉

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