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

Feat: Add flag aliases feature #73

Merged
merged 1 commit into from
Feb 9, 2025
Merged

Conversation

oNaiPs
Copy link
Contributor

@oNaiPs oNaiPs commented Dec 4, 2024

@SupImDos first of all, thanks for such a useful project!

This PR attempts to implement #44.
It seems to have the functionality, however I'd like your guidance regarding next steps:

  • What do you think of the implementation approach?
  • Docs
  • More Tests

Some comments regarding these edge cases:

What happens if you provide alias and aliases?
What is the behaviour for boolean arguments
What is the behaviour for inverted arguments (i.e., --no-<x>)

It seems to me that alias and aliases target different cases. Since alias maps to argparse's dest field, the optional aliases flags you set up would not alter this behavior.
In regards to the boolean option, BooleanOptionalAction should take care of it (if option starts with --, negates)
Finally, regarding inverted arguments, not sure what would be the case here, could you give an example?

Closes #44.

@oNaiPs oNaiPs marked this pull request as draft December 4, 2024 11:52
@oNaiPs
Copy link
Contributor Author

oNaiPs commented Feb 8, 2025

Ping @SupImDos any chance you could look at this?

@SupImDos
Copy link
Owner

SupImDos commented Feb 9, 2025

Ping @SupImDos any chance you could look at this?

Hi @oNaiPs, somehow I didn't see this! This looks great, I'll have a look at getting it merged and released ASAP.

@SupImDos
Copy link
Owner

SupImDos commented Feb 9, 2025

@oNaiPs

What do you think of the implementation approach?

Looks great, this is pretty much how I was thinking it would be done.

Docs
More Tests

I think the easiest thing to do here is merge what you've got here into master, and I'll create a follow-up PR to address these. Hope that's okay!

@SupImDos SupImDos marked this pull request as ready for review February 9, 2025 07:11
@SupImDos SupImDos merged commit 977bc47 into SupImDos:master Feb 9, 2025
SupImDos added a commit that referenced this pull request Feb 9, 2025
This is a follow-up PR to #73 which aims to address any bugs, add unit tests and add documentation.
@SupImDos
Copy link
Owner

SupImDos commented Feb 9, 2025

@oNaiPs I did a bit of tweaking in #75 and released this feature in v0.10.0. Hope this is what you were looking for!

@oNaiPs
Copy link
Contributor Author

oNaiPs commented Feb 9, 2025

@oNaiPs I did a bit of tweaking in #75 and released this feature in v0.10.0. Hope this is what you were looking for!

Thank you so much @SupImDos ! Yes most definitly!

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.

Add possiblity to address the same option via several alias names
2 participants