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 common short version for help of CLI #509

Merged
merged 1 commit into from
Apr 16, 2024

Conversation

anotherbridge
Copy link
Contributor

Added a short version to call the help, i.e. now the help of a functionality is available by calling --help and -h.

Changes

  • Added help option names in the context settings of the click group added to the nitropy command.

Checklist

  • run make check
  • run make fix
  • tested with Python3.9
  • signed commits
  • updated documentation (e.g. parameter description, inline doc, docs.nitrokey)
    • Nothing to update in my point of view, please correct me if I'm wrong.
  • added labels

Test Environment and Execution

  • OS: macOS Sonoma 14.3.1
  • device's model: Nitrokey 3C NFC
  • device's firmware version: v1.6.0-test.20231218

@daringer
Copy link
Collaborator

daringer commented Apr 12, 2024

not sure if it's a good idea to apply make fix here, as obviously change is not really atomic anymore - what do you think @sosthene-nitrokey or @robin-nitrokey ?

@robin-nitrokey
Copy link
Member

I think it’s fine. The commits are atomic, and atomic PRs is something that is neither achievable nor even desirable.

Though I wonder why these changes occur in the first place – some linting dependency that we didn’t pin hard enough? New Python version?

@sosthene-nitrokey
Copy link
Contributor

I think we should manually cherry-pick a0ce779 instead of merging.

@robin-nitrokey
Copy link
Member

I’m really confused now. Why can’t we start a CI run for this PR? And why did make fix perform these changes in the first place? When testing locally, make check fails with these changes, so we should not merge them.

@daringer
Copy link
Collaborator

daringer commented Apr 12, 2024

weird, I would also expect that the CI should run this....

but, ok let's only include the first commit, @anotherbridge can you be so kind and remove the make fix commit, just include a0ce779 in this PR please - think something went wrong with your python version maybe ?

@robin-nitrokey
Copy link
Member

For first-time contributors, CI runs need to be approved. I did that recently, but I don’t remember if it was for this PR. Anyway, either the CI run should show up, or the option to approve and trigger it.

@anotherbridge
Copy link
Contributor Author

@daringer @robin-nitrokey @sosthene-nitrokey I removed the commit that includes the make fix and also merged your master branch into my fork.

@robin-nitrokey robin-nitrokey merged commit bd61327 into Nitrokey:master Apr 16, 2024
4 checks passed
@robin-nitrokey
Copy link
Member

Rebased and merged. Thank you!

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.

4 participants