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

Change author/about/version behavior #229

Merged
merged 3 commits into from
Aug 23, 2019
Merged

Change author/about/version behavior #229

merged 3 commits into from
Aug 23, 2019

Conversation

CreepySkeleton
Copy link
Collaborator

Closes #217

@CreepySkeleton
Copy link
Collaborator Author

I decided not to create a new PR for the raw check, I affects the same lines that the first commit does, I'd have to stack my work on top of it anyway

@CreepySkeleton
Copy link
Collaborator Author

Interesting...

@CreepySkeleton
Copy link
Collaborator Author

CreepySkeleton commented Aug 23, 2019

@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 cargo test is flawless and shining. And yes, I know about the warning, will fix in next push

@CreepySkeleton
Copy link
Collaborator Author

My travis build can't reproduce it. Any idea?

@TeXitoi
Copy link
Owner

TeXitoi commented Aug 23, 2019

Strange...

@CreepySkeleton
Copy link
Collaborator Author

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

@TeXitoi
Copy link
Owner

TeXitoi commented Aug 23, 2019

I can't find anything to download. Any pointer?

CHANGELOG.md Outdated Show resolved Hide resolved
structopt-derive/src/attrs.rs Outdated Show resolved Hide resolved
structopt-derive/src/attrs.rs Outdated Show resolved Hide resolved
@@ -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();
Copy link
Owner

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?

Copy link
Collaborator Author

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...

Copy link
Collaborator Author

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.

@CreepySkeleton
Copy link
Collaborator Author

CreepySkeleton commented Aug 23, 2019

I can't find anything to download. Any pointer?

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

@hellow554
Copy link

My travis build can't reproduce it. Any idea?

Looks like an incremental bug. I think it is covered in rust-lang/rust#63161, you can leave a comment there.

@CreepySkeleton
Copy link
Collaborator Author

My travis build can't reproduce it. Any idea?

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 :-)

@CreepySkeleton
Copy link
Collaborator Author

Also, I squashed all the commits into one, tell me if you want to separate them

@TeXitoi
Copy link
Owner

TeXitoi commented Aug 23, 2019

I squash at merge time. That's simpler if I can review the modifications added, thanks.

@CreepySkeleton
Copy link
Collaborator Author

May I ask why do you always squash?

@TeXitoi
Copy link
Owner

TeXitoi commented Aug 23, 2019

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.

@CreepySkeleton
Copy link
Collaborator Author

CreepySkeleton commented Aug 23, 2019

Yeah, I do understand why people squash meaningless commits like oops, penis and stuff - but I also find useful to separate related but different things of one giant commit into separate ones. For example, if it was me in your shoes I'd probably break this commit into three - Change behavior..., Add raw removal warning and Update Changelog. Sometimes feature implementing may grow into one pretty monstrous commit and it's easier to shatter not-really-related things into smaller pices. IMHO of course...

@TeXitoi
Copy link
Owner

TeXitoi commented Aug 23, 2019

If you do that, I'll just rebase, not squash

@TeXitoi
Copy link
Owner

TeXitoi commented Aug 23, 2019

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
keep the history with these 2 commits. I will not take the time to separate these 2 commits myself.

@CreepySkeleton
Copy link
Collaborator Author

but if you separate this work in 2 commit, I'll happily
keep the history with these 2 commits. I will not take the time to separate these 2 commits myself.

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.

@TeXitoi TeXitoi merged commit 2363815 into TeXitoi:master Aug 23, 2019
@TeXitoi
Copy link
Owner

TeXitoi commented Aug 23, 2019

Thanks!

@CreepySkeleton
Copy link
Collaborator Author

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 :)

@CreepySkeleton CreepySkeleton deleted the 0.3.0 branch August 23, 2019 13:13
@TeXitoi
Copy link
Owner

TeXitoi commented Aug 23, 2019

Sorry no, just looked at the status

@sourcefrog
Copy link

sourcefrog commented Apr 28, 2020

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 author explicitly mentioned." It was a little confusing.

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.

Stop doing automatic author, about and version
4 participants