Skip to content
This repository has been archived by the owner on Jun 2, 2020. It is now read-only.

--name should work both as a positional arg and as an option arg #403

Merged
merged 6 commits into from
Oct 2, 2019

Conversation

ajnirp
Copy link
Contributor

@ajnirp ajnirp commented Aug 18, 2019

This is enabled by creating a separate, required, positional arg and making it conflict with the original optional arg. I applied this change to each subcommand of cargo func new.

What is the goal of this pull request?

It addresses #374

What does this pull request change?

A user creating a new Azure Function can leave out the --name option while doing so. They must still provide a name, as before.

What work remains to be done?

None.

Do you consider it adequately tested?

Yes - I reinstalled the SDK with cargo install, then tested new behaviour with these commands:

  1. cargo func new help http
  2. cargo func new http abc --auth-level anonymous
  3. cargo func new http --auth-level anonymous abc
  4. cargo func new http --auth-level anonymous --name abc

Related Issues

None

Notes

N.A.

@cla-bot cla-bot bot added the cla-signed The CLA has been signed by the contributor. label Aug 18, 2019
@ajnirp
Copy link
Contributor Author

ajnirp commented Aug 26, 2019

@peterhuene thoughts?

name: args.value_of("name").unwrap(),
name: args.value_of("positional-name").unwrap_or_else(|| {
args.value_of("name")
.unwrap_or("Default fallback - never reached")
Copy link
Owner

Choose a reason for hiding this comment

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

Can this be an expect instead?

Copy link
Owner

Choose a reason for hiding this comment

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

If so, this comment applies to each command.

Copy link
Owner

Choose a reason for hiding this comment

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

Hi @ajnirp. Sorry for the delay in following up with you. I'd like to get this merged soon. Did you see this review question, namely changing unwrap_or to expect? Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The CI checks for cargo fmt -- --check and cargo clippy --release -- --Dwarnings are failing, but those same commands are working (i.e. $? is 0) for me locally. Any idea what I might be missing?

Copy link
Owner

Choose a reason for hiding this comment

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

The clippy error is from generated code that we can ignore for now.

The rustfmt one is odd. The CI uses the latest stable, so if you're also using the latest stable I can't explain the difference.

Let me look into this tonight if I can; I won't block merging your PR for a format / linter error, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, thanks!

@peterhuene
Copy link
Owner

Hi @ajnirp. Sorry I was on vacation last week. The changes look great!

I just have the one question. Thanks again for fixing this issue!

Copy link
Owner

@peterhuene peterhuene left a comment

Choose a reason for hiding this comment

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

I think we should change unwrap_or with a dummy value to expect.

Other than that, looks great!

@ajnirp
Copy link
Contributor Author

ajnirp commented Sep 26, 2019

Apologies for the late reply - I'll get around to it this weekend! I have no access to a personal machine for coding until then. Thank you for the review!

This is enabled by creating a separate, `required`, positional arg and making it conflict with the original optional arg. I applied this change to each subcommand of `cargo func new`.
This commit fixes the formatting of `binding.rs` with the latest stable
toolset.
This commit ignores the lint warning for type repetition in bounds for the gRPC
generated code.
@peterhuene
Copy link
Owner

It looks like the only failure is the nightly rustfmt trying to undo the formatting changes for the stable compiler.

As such, I'm going to ignore that failure.

@ajnirp thanks very much for the contribution!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla-signed The CLA has been signed by the contributor.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants