Skip to content

Conversation

@AlexKVal
Copy link
Owner

AlexKVal added a commit that referenced this pull request Sep 17, 2015
Now 'dry run' is default mode. To prevent accidental mistakes.
@AlexKVal AlexKVal merged commit 0d5b1c5 into master Sep 17, 2015
@AlexKVal AlexKVal deleted the human-factor branch September 17, 2015 06:09
@jquense
Copy link
Contributor

jquense commented Sep 17, 2015

hey! thanks for all the recent work on this. Could we maybe make that default configurable? I understand the reason for such a default but its counter to how shell scripts work for almost everything else. Having to type the additional --run for 90% of the times I am running this is a bit annoying

@AlexKVal
Copy link
Owner Author

No problem. I agree that it is counter to all other scripts, but this is exactly the feature 😄
We release software very rare. And time goes - we forget about those -- gouble dashes or smth else.

I've made it such way that it reminds about "--run" option:
screen shot 2015-09-17 at 9 55 59 am

you instantly press Ctrl+C, arrow up add --run and press Enter 😉

If you still think with all of my arguments 😜 that "--dry-run" option and configurable default behavior are needed, I will add them to release-script 🍒

What do you think ?

@jquense
Copy link
Contributor

jquense commented Sep 17, 2015

I still find the default counter intuitive, the script if for releasing, not for seeing if a release would work. Git also has a dry-run mode, but that isn't the default for a reason, it would be really hard to work with if it did. I am almost always running the script to release my module so I usually expect that to be the default behavior. Forgetting, having to ctrl-c, arrow up and type --run is pretty bad UX, when I should just be able to type the command. If people want safety then the general procedure is to alias the command with --dry-run always on, but I wouldn't make it the default.

:) I do appreciate the attempt to try and prevent mistakes, but in this case I think its over prescriptive and ends up not actually being helpful most of the time. Especially since if you accidentally release it really isn't that hard to undo do it.

@AlexKVal
Copy link
Owner Author

Heh. Ok ok 😄

Then what is the best way to address it in release-script you think ?

I would prefer to leave it as a feature of release-script to run in "dry run" mode by default.

Then it seems I need to add option smth like that: "defaultDryRun": "false" ?

@AlexKVal
Copy link
Owner Author

And then what do you think about react-bootstrap/react-prop-types#16 ?
It seems that PR is superfluos. Close it ? Or wait others to say their opinions ?

@jquense
Copy link
Contributor

jquense commented Sep 17, 2015

that can be up to others since I don't usually work on that project :)

@AlexKVal
Copy link
Owner Author

But you don't mind about "defaultDryRun": "false" option in package.json then ?

@jquense
Copy link
Contributor

jquense commented Sep 17, 2015

sure seems fine :) I'd of course prefer that the default be false as well, but it is your project so I bow to whatever you think is the best default. I am happy as long as I can configure it at least

@AlexKVal
Copy link
Owner Author

Ok then. I'll add it 🍒

Truth is I am also made those accidental publishings... and worse of that, they were partial..
I did forget to add bower option and it had released only npm package.
The next patch version has been with empty changelog because I still needed to publish bower version.

Then I did forget to add GitHub Token in my console after reboot 😄
And again I had to fix it manually.

That is my main motivation for such haste to add and release "dry run" default mode.
@taion by that comment has nudged my thoughts 😄

@taion
Copy link

taion commented Sep 17, 2015

Thinking about this a bit more, I agree w/@jquense - I think it'd be really valuable to make these scripts be able to default to dry runs, but that the default setting for this config option should probably be to release, just to make things as straightforward as possible.

We'd probably want to configure things to go through dry runs, but one extra line of config seems like not too big a deal.

@AlexKVal
Copy link
Owner Author

Would it be too much if I then reverse 'defaultDryMode' to 'actual run' (as it was before) in the next minor release-script@0.6.0 version ?
And it will be with this new defaultDryMode option set to false by default, but will be documented
for those who wish to change default behavior.
While now there are a very little projects / users of r-script I presume it woun't be too much deal ?

What do you think guys ?

And second question: I presume I could safely just close these two PRs ?
react-bootstrap/react-bootstrap#1329
react-bootstrap/react-prop-types#16

@AlexKVal
Copy link
Owner Author

Or I can leave defaultDryRun mode as true and just change those two PR's with adding:

"release-script" : {
  "defaultDryRun": "false"
}

?

@taion
Copy link

taion commented Sep 17, 2015

I'm not sure I understand. I think we should just change 136c511#diff-a1ca48439c82c10c6224f7c126531cf8R52 to do === true. I think it'd be nice to have this option enabled for React-Bootstrap because I'm afraid of accidentally publishing.

@AlexKVal
Copy link
Owner Author

I think it'd be nice to have this option enabled for React-Bootstrap because I'm afraid of accidentally publishing.

Then just merge react-bootstrap/react-bootstrap#1329 as is 😉
It is exactly for your case.
"Dry-run" mode by default.
You have to add --run when you are ready to actually release React-Bootstrap.

And then I propose to merge react-bootstrap/react-prop-types#16 too.
It will have "dry-run" mode by default too.

@taion
Copy link

taion commented Sep 18, 2015

Okay - we'll make a note to upgrade again once you have a new release.

@AlexKVal
Copy link
Owner Author

For posterity: (from my private gitter chat with @taion)

I've decided to leave it now with dry run mode by default.
If in the future this will be an issue for new users - they will write about it and write a lot 😄
And then I always can switch default mode other way.

🍒

@AlexKVal AlexKVal mentioned this pull request Jan 13, 2016
@mjhasbach
Copy link
Contributor

I also agree that dry run should not be the default.

@AlexKVal
Copy link
Owner Author

Ok then. It seems I should change the default mode ❇️
Probably I should release it as a major version change. In this case 1.0.0.

AlexKVal added a commit that referenced this pull request Jan 13, 2016
#19 (comment)

The '--run' command line option and "defaultDryRun" have been removed.
@AlexKVal
Copy link
Owner Author

Version 1.0.0 is out.

@taion
Copy link

taion commented Jan 13, 2016

Interesting – you didn't want to leave the option and just change the default?

@AlexKVal
Copy link
Owner Author

I thought (from that discussion) that nobody needs the "defaultDryRun" option in package.json so I totally removed it.
Do you think it is worth it to leave it ?

@taion
Copy link

taion commented Jan 13, 2016

I think it's a valuable option. I'd turn it on for e.g. React-Bootstrap. I don't think it should be the default, though.

@AlexKVal
Copy link
Owner Author

O.. I see. Ok then I'll put it back 👍
Thank you for pointing to it out ❇️

AlexKVal added a commit that referenced this pull request Jan 13, 2016
#19 (comment)

It seems there had been some confusion on my side :)
AlexKVal added a commit that referenced this pull request Jan 13, 2016
#19 (comment)

It seems there had been some confusion on my side :)
AlexKVal added a commit that referenced this pull request Jan 13, 2016
#19 (comment)

It seems there had been some confusion on my side :)
@AlexKVal
Copy link
Owner Author

I have put it back #39 😊
v1.0.1

@taion
Copy link

taion commented Jan 13, 2016

Shouldn't this be 1.1.0, since it's a new(-ish) feature? 😆

@AlexKVal
Copy link
Owner Author

I decided to use patch because in the reality the feature was absent for a couple of hours 😆
and there is no breaking changes after 1.0.0 introduced by it.

Though I admit that I should have used
MINOR version when you add functionality in a backwards-compatible manner (http://semver.org/) probably.
It is my first MAJOR release ever, bear with me. 😊

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.

5 participants