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

Backwards-incompatible change made. #738

Closed
jrvanwhy opened this issue Nov 12, 2016 · 14 comments
Closed

Backwards-incompatible change made. #738

jrvanwhy opened this issue Nov 12, 2016 · 14 comments

Comments

@jrvanwhy
Copy link

Commit 1ced2a7 introduced a " + Clone" trait bound to many functions, including App::get_matches_with(), which is a breaking change and technically requires a major version increase.

We should see if this bound can be removed without losing functionality (which would obviously be another breaking change).

jrvanwhy referenced this issue Nov 12, 2016
…l argument as multiple(true)

Now one can build CLIs that support things like `mv <files>... <target>`

There are a few requirements and caveats;

 * The final positional argument (and all positional arguments prior) *must* be required
 * Only one positional argument may be `multiple(true)`
 * Only the second to last, or last positional argument may be `multiple(true)`

Closes #725
@jrvanwhy
Copy link
Author

I think I have a workable solution, but it's hanging the compiler. Unfortunately, I need to get permission from my employer before releasing my changes, so this will take a bit of time. Assuming I am correct in thinking I have a solution, I will have a patch ASAP, but that may be a few days.

@kbknapp
Copy link
Member

kbknapp commented Nov 12, 2016

Thanks for filing this! And also thanks for working on this issue! Let me know if you have any questions.

@jrvanwhy
Copy link
Author

Thanks for the offer of help. I've looked at my employer's (Google's) documentation, and it looks like they will own the copyright on the patch I'm creating. I notice that the current codebase is copyrighted solely by you. Are you fine accepting patches that would make you share copyright with Google in those two files? (It'd still be under the MIT license, of course).

If not, then my suggestion is to remove the Clone bound from get_matches_from(), get_matches_from_safe(), and get_matches_from_borrow() in app/mod.rs and change the iterator in get_matches_with() in parser.rs to create OsString objects with than T: Into objects. I'm not sure if that works, however, because I'm hitting a compiler bug (currently waiting on Google's permission to release the patch so the Rust team can investigate the issue).

@kbknapp
Copy link
Member

kbknapp commented Nov 12, 2016

@jrvanwhy I'm not against sharing the copyright, but being that I'm only one guy working on a side project that would be alittle more complicated than I'd want to think/worry about right now since I'm nowhere even close to being a lawyer 😜

Let me see if I can get this working - if not then I'll concede and take the patch!

@jrvanwhy
Copy link
Author

I understand completely, sorry my patch-making is tied up in bureaucracy. Good luck making it work, and thank you for supporting this project so well!

@tormol
Copy link
Contributor

tormol commented Nov 13, 2016

@kbknapp I'm almost certain you already share the copyright.
Copyright reassignment isn't mentioned anywhere, and that that stuff seems to require paperwork.
The copyright notice in lib.rs says '... and clap-rs contributors', so either that or LICENCE-MIT is wrong.
If you were the sole copyright holder the contributor checkoff for re-licensing would be unnecessary.

@kbknapp
Copy link
Member

kbknapp commented Nov 14, 2016

@tormol you are probably correct, which is in part why I tend to steer clear of copyright paperwork unless it's required since it's a murky field.

I think what I'm most leery of is singling out a particular contributor for individual lines/files. Because what happens when those particular parts change, do the derivative works also need to added to this list of singled out contributions? That would be hard to keep up correctly. Granted this isn't to say any one of the clap contributors could request the same thing, but most are good with having their name listed in the contributors and using things like git blame.

With individual github contributors this is typically hand waved over, but when big corporation get involved things to be done correctly or it gets messy quick. Since I'm unfamiliar with how that whole derivative work part works, that's what I'm unsure of. The way I understand it sounds like a virus that spreads to all things derived from those original few lines?

I'm all for sharing the love with the contributors, because without all of them, clap wouldn't be what it is today! I just don't want to do something totally wrong and get hung out to dry by mistake!

I could be waaay off too, so take what I say with a grain of salt 😜

@jrvanwhy
Copy link
Author

jrvanwhy commented Nov 14, 2016

@kbknapp The following might work, but I would need to clear it with Google first:

  • I replace "Kevin B. Knapp" with "Kevin B. Knapp and other contributors" in LICENSE-MIT
  • I add Google, Inc to CONTRIBUTORS

Would that set of changes be fine with you? Of course, don't feel like you need to wait for me to make changes -- if you find a solution on your own, go ahead and commit it. I'm just trying to save you some effort by solving it myself ;p

@jrvanwhy
Copy link
Author

The above changes are fine with Google. However, I'm not sure how to make the "Google, Inc" persistent in CONTRIBUTORS, since CONTRIBUTORS is apparently regularly regenerated from commit messages.

TimNN pushed a commit to TimNN/clap-rs that referenced this issue Nov 15, 2016
…e backwards-compatibility without a major version number increment), plus a LICENSE file fix to give correct copyright attribution.

I am unsure how to add Google Inc to CONTRIBUTORS.md given that CONTRIBUTORS.md is autogenerated. We should find a way to include Google Inc into CONTRIBUTORS.md instead of LICENSE-MIT.
@kbknapp
Copy link
Member

kbknapp commented Nov 15, 2016

@jrvanwhy absolutely, I was going to do that regardless (well similar, I was going to list your name 😉). Assuming it's good with your employer I'd still like to add your name to the CONTRIBUTORS.md list. After making that change, do you know who from Google can give the thumbs up/down to #389 ? That change would allow this library to be compatible with the Rust ecosystem at large. As @tormol pointed out, that thumbs up/down may not even be totally necessary, but in case it is I'd still like to know who can give that approval.

@jrvanwhy
Copy link
Author

@kbknapp Unfortunately, Google requires that I add "Google Inc" as a contributor, which is difficult because your contributors list is auto-generated and the tool you're using would only pick up my username. The patch I posted (on Reddit) for the compiler devs changed LICENSE-MIT to mention Google Inc, which I find a bit heavy-handed.

Assuming I am able to create a working patch, I will ask Google's licensing team how to proceed. My guess is that I should release the patch under the proposed dual license.

@jrvanwhy
Copy link
Author

@kbknapp Here's an idea: What if I modified justfile to append something like:

`The following entities have contributed to clap-rs:

  • Google Inc`

to CONTRIBUTORS.md so that you can maintain a list of companies/nonprofit organizations/governments, etc... that have contributed to clap-rs?

Also, I'm still blocked on the rustc bug I found.

@kbknapp
Copy link
Member

kbknapp commented Nov 21, 2016

I'm good with that too 👍

@vitiral
Copy link

vitiral commented Nov 26, 2016

I just hit this bug and it was a quick fix. I just want to let you know how grateful I am for this library. It is an amazing command line lib and you are awesome for maintaining it. 👍

@kbknapp kbknapp closed this as completed Feb 2, 2018
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

No branches or pull requests

4 participants