Skip to content

Comments

Feature/required args#66

Merged
alanag13 merged 10 commits intomasterfrom
feature/required-args
May 18, 2020
Merged

Feature/required args#66
alanag13 merged 10 commits intomasterfrom
feature/required-args

Conversation

@alanag13
Copy link
Contributor

@alanag13 alanag13 commented May 15, 2020

redo the internals of the cli so that only commands that have exactly one required argument use positional arguments.

commands with multiple required arguments will now be flagged. profile create is the only command affected by this change.

@antazoey
Copy link
Contributor

I think code42 high-risk-employee add-risk-tags and remove-risk-tags might also be affected


### Changed

- `code42 profile create` now uses required `--name`, `--server` and `--username` flags instead of positional arguments.
Copy link
Contributor

Choose a reason for hiding this comment

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

Just confirmed. code42 high-risk-employee add-risk-tags and remove-risk-tags have also changed.

Copy link
Contributor

Choose a reason for hiding this comment

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

And the usage for adding / removing risk-tags

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thank. you

self._settings[u"nargs"] = nargs

def set_required(self, required=False):
def set_required(self, required):
Copy link
Contributor

Choose a reason for hiding this comment

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

This method could probably just be make_required()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was already in place -- and you could use it to set something that would normally be required to not be required. Keeping as-is seems more flexible imo.

# this is a positional arg, treat it as a required cli arg.
option_names = [param_name]
return ArgConfig(*option_names, default=default)
if num_positional_args > 1:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you could do an elif and an else for the parent block instead of a nested if block here.

required = True
else:
option_names = [param_name]
return ArgConfig(*option_names, default=default, required=required)
Copy link
Contributor

Choose a reason for hiding this comment

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

I am trying to test the single arg case to make sure that still works but it seems to now. I can't do code42 high-risk-employee add juliya.smith+partners@code42.com

It is making me do code42 high-risk-employee add --username juliya.smith+partners@code42.com

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmmm thanks, I'll look into this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is because im not excluding sdk and profile, good find, thanks

Copy link
Contributor

@antazoey antazoey left a comment

Choose a reason for hiding this comment

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

Some stuff is not working yet. I am happy we're doing this.

Command(
u"add-risk-tags",
u"Associates risk tags with a user.",
u"code42 high-risk-employee add-risk-tags --username <username> --risk-tag <risk-tags>",
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think code42 high-risk-employee add-risk-tags --username --tags <risk-tags would be better? difference between it's less redundant than saying risk-tag over again. The word risk happens three times for this command

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah i do like that

u"Create profile settings. The first profile created will be the default.",
u"{} {}".format(usage_prefix, u"create <profile-name> <server-address> <username>"),
u"{} {}".format(
usage_prefix, u"create --name <name> --server <server-address> --username <username>"
Copy link
Contributor

Choose a reason for hiding this comment

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

Would name confuse anyone? Would anyone mistake this for their human name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i can switch the <name> to <profile-name> but dont think the flag name has to change

@alanag13 alanag13 merged commit f92947f into master May 18, 2020
@alanag13 alanag13 deleted the feature/required-args branch June 3, 2020 15:18
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