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

Optional argument #53

Merged
merged 2 commits into from
Aug 28, 2017
Merged

Conversation

tromey
Copy link
Contributor

@tromey tromey commented Aug 28, 2017

No description provided.

tromey added 2 commits August 28, 2017 02:59
test_optflagopt did not actually test optflagopt.
This changes the parsing of long options that have optional arguments.
Now the `=` is required; that is, `--arg=something` and `--arg something`
are parsed differently.
Fixes rust-lang#49
@alexcrichton alexcrichton merged commit 40a73df into rust-lang:master Aug 28, 2017
@alexcrichton
Copy link
Member

Thanks!

@nox
Copy link

nox commented Aug 24, 2018

That's a breaking change and should have been bumped as such.

@tromey tromey deleted the optional-argument branch August 24, 2018 14:31
SimonSapin added a commit to servo/servo that referenced this pull request Aug 24, 2018
servo-wpt-sync pushed a commit to servo-wpt-sync/web-platform-tests that referenced this pull request Aug 24, 2018
SimonSapin added a commit to servo/servo that referenced this pull request Aug 24, 2018
servo-wpt-sync pushed a commit to servo-wpt-sync/web-platform-tests that referenced this pull request Aug 24, 2018
jdm pushed a commit to web-platform-tests/wpt that referenced this pull request Aug 24, 2018
@KodrAus
Copy link
Contributor

KodrAus commented Aug 26, 2018

Identifying logically breaking changes like this is pretty tricky. Just looking at the diff here I can't exactly pinpoint what breakage occurs, but I suspect just about any change to existing behaviour is potentially going to break someone.

@nox do you have any more light to shed or suggestions on how we could manage behavioural changes better?

@SimonSapin
Copy link
Contributor

I didn’t look at the diff, but the change of behavior is explained in a commit message:

This changes the parsing of long options that have optional arguments.
Now the = is required; that is, --arg=something and --arg something
are parsed differently.

This means that a script that passed --arg something to a program that uses getopts 0.2.14 will break (not have the intended behavior anymore) when the program updates to getopts 0.2.15.

In servo/servo#21356, this manifested as error: Service is unavailable: 127.0.0.1:4444 in the web-platform-tests harness.

Hindsight makes saying this much easier to say of course, but I think it should have been readily apparent from the change description that such breakage was possible. It would have been good to discuss that potential in this PR before landing it. Even a single sentence along the lines of “I think breakage will be minimal enough to be acceptable because X and Y” would have shown that this was considered. Although in this case (again, with hindsight) it would have been preferable to conclude that this change should not land without a semver-incompatible version number increment, 0.3.0. (However 0.2.15 has been out long enough that likely doesn’t help to do that increment now.)

@KodrAus
Copy link
Contributor

KodrAus commented Aug 26, 2018

Thanks @SimonSapin, that's a fair assessment. Funnily enough the commit message is the place I didn't look...

We should try to be more mindful of capturing the rationale behind, and weighing up the factors leading to, the landing of any changes.

zcorpan pushed a commit to web-platform-tests/wpt that referenced this pull request Aug 27, 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

Successfully merging this pull request may close these issues.

None yet

5 participants