-
Notifications
You must be signed in to change notification settings - Fork 152
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
Change author/about/version
behavior
#229
Conversation
I decided not to create a new PR for the |
Interesting... |
@TeXitoi Could you please take a look? It seems like travis crashes on all channels with some compiler-bug error. I can't reproduce it locally, my |
My travis build can't reproduce it. Any idea? |
Strange... |
Yahoo! Now travis is all green just like a troll from 4chan. Can I ask you to download and share build artifacts of this particular build with me? I'd like to dig into this bug or at least report it to rustc team |
I can't find anything to download. Any pointer? |
structopt-derive/src/lib.rs
Outdated
@@ -282,7 +283,7 @@ fn gen_clap(attrs: &[Attribute]) -> GenOutput { | |||
let attrs = Attrs::from_struct(attrs, Sp::call_site(name), Sp::call_site(DEFAULT_CASING)); | |||
let tokens = { | |||
let name = attrs.cased_name(); | |||
let methods = attrs.methods(); | |||
let methods = attrs.clone().top_level_methods(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are these clone needed now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i just realized that this is quite complicated question, let me think...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my previous attempt these methods mutated self
. Hence we happen to reuse attrs
here first call would add methods that are acceptable for clap::App
but not for clap::Arg
. First we used this to generate App
and later we generated Arg
. My current approach is different, these methods mutate self no longer.
I don't know, I can't find any way to do that in google so I asked the overmind, let's see what we can do |
Looks like an incremental bug. I think it is covered in rust-lang/rust#63161, you can leave a comment there. |
Hmm, I like to leave comments, especially on my second vacation day :-) |
Also, I squashed all the commits into one, tell me if you want to separate them |
I squash at merge time. That's simpler if I can review the modifications added, thanks. |
May I ask why do you always squash? |
One commit, one feature. And some contributors do a lot of commit as "fix typo", "fmt", "oups, bug fix" that can be annoying in a git bisect. |
Yeah, I do understand why people squash meaningless commits like |
If you do that, I'll just rebase, not squash |
I don't want to put pressure on contributors about managing commits in a certain way, but I might ask for a separate PR for unrelated stuff. I allowed you to do one PR for raw and change behavior, thus I will not ask you to do 2 PR, but if you separate this work in 2 commit, I'll happily |
Exactly what I would do, except I'd probably just isolate these 2 commits myself. N.B: I'm not actually arguing, I just noticed some policy that is different from mine and got curious about it's advantages and drawbacks. That's how people learn, and I'm trying. |
Thanks! |
Mm, @TeXitoi did you look at travis output? I'm asking because it seems we missed a warning, I just was about to push the fix when you merged it :) |
Sorry no, just looked at the status |
It seems like the docs are out of date with this? They seem to still indicate, in https://docs.rs/structopt/0.3.14/structopt/#magical-methods that these default to the Cargo values. Edit: Oh, no, I see it says "only when |
Closes #217