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

Add interactive UI #97

Merged
merged 7 commits into from Nov 4, 2016
Merged

Conversation

zinserjan
Copy link
Contributor

@zinserjan zinserjan commented Oct 9, 2016

This PR adds a interactive ui when np runs without arguments as suggested in #52.

features:

  • choose increment type (e.g. patch) or specify own version (e.g. 0.1.3)
  • choose NPM dist-tag for pre-releases (except packages with "private": true)
    • user can choose from a list of already existing tags for this package or
    • specifiy a custom tag except latest
  • show info about which version you're going from and to.
  • user have to confirm the release to prevent accidental publishing of wrong versions

Edit(sindresorhus):

You can install this PR with: $ npm i -g 'zinserjan/np#interactive-ui' and run it with $ np.

screen shot 2016-10-11 at 03 03 21

* choose increment type or specify own version
* choose NPM dist-tag for pre-releases
* private packages don't need dist-tags
* version helper for shared stuff
@@ -2,14 +2,16 @@
'use strict';
const meow = require('meow');
const updateNotifier = require('update-notifier');
const version = require('./lib/version');
const interactiveui = require('./lib/interactiveui');
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just call the file and import: ui

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do mean exacly with "just call the file and import ui" ?

const ui = require('./lib/interactiveui');

this?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const ui = require('./lib/ui');

const np = require('./');

const cli = meow(`
Usage
$ np <version>

Version can be:
patch | minor | major | prepatch | preminor | premajor | prerelease | 1.2.3
${version.SEMVER_INCREMENTS.join(' | ')} | 1.2.3

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should have a description here that running just np starts the interactive UI.


exports.satisfies = function satisfies(version, range) {
return semver.satisfies(version, range);
};
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use arrow functions (inline ones when possible) for all these.

@@ -0,0 +1,114 @@
import test from 'ava';
import {SEMVER_INCREMENTS, PRERELEASE_VERSIONS, isValidVersionInput, isPrereleaseVersion, getNewVersion, isVersionGreater, isVersionLower, satisfies} from '../lib/version';
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Too verbose. Just import version and use them as properties.

value: null
}
]),
filter(input) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use inline arrow function.

filter => version.isValidVersionInput(input) ? version.getNewVersion(pkg.version, input) : input,

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same for all the other prompt functions.

return !pkg.private && version.isPrereleaseVersion(answers.version) && !options.tag && !answers.tag;
},
validate(input) {
if (!input.length) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (input.length === 0) {

name: 'confirm',
message(answers) {
const tag = answers.tag || options.tag;
const msg = `Will bump from ${pkg.version} to ${answers.version}${tag ? ` and tag this release in NPM as ${tag}` : ''}. Continue?`;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NPM => npm

{
type: 'list',
name: 'tag',
message: 'How should this pre-release version be tagged in NPM?',
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NPM => npm

},
validate(input) {
if (!version.isValidVersionInput(input)) {
return 'Please specify a valid semver, e.g. 1.2.3. See http://semver.org/';
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

http://semver.org/ => http://semver.org

type: 'list',
name: 'version',
message: 'Select semver increment or specify new version',
choices: version.SEMVER_INCREMENTS.concat([
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should show what the version would end up being with each increment:

patch (2.3.1)
minor (2.4.0)
major (3.0.0)

And the (2.3.1) should be dimmed with chalk.gray.dim(text).

https://github.com/chalk/chalk

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea.

@sindresorhus
Copy link
Owner

Wow. ❤️ This is a really good pull request!

@sindresorhus sindresorhus changed the title interactive ui when no version is provided Add interactive UI Oct 9, 2016
@sindresorhus
Copy link
Owner

Can you also document the interactive UI feature in the readme? Could have a new top-level section about it.

I think it would be useful to see the current version before the Inquirer questions:

$ np
Current version: 1.4.3
? Select semver increment or specify new version (Use arrow keys)
❯ patch 
  minor 
  major 

What do you think?

@sindresorhus
Copy link
Owner

choose NPM dist-tag for pre-releases (except private package)

Why except for private packages?

@SamVerschueren
Copy link
Collaborator

I think it would be useful to see the current version before the Inquirer questions

Could be useful, but necessary if we show the new version after patch, minor and major? Just wondering though :).

@sindresorhus
Copy link
Owner

@SamVerschueren It gives a clear picture of where you're at without having to calculate backwards.

@SamVerschueren
Copy link
Collaborator

Fair point :)

@zinserjan
Copy link
Contributor Author

Can you also document the interactive UI feature in the readme? Could have a new top-level section about it.

I think a gif like in the intro section would be cool. How do you created that?

I think it would be useful to see the current version before the Inquirer questions

Makes sense for me.

Why except for private packages?

Private packages are not published to npm, that's why we don't need the "choose your dist-tag" step.
Have a look at this already existing code in index.js

@sindresorhus
Copy link
Owner

I think a gif like in the intro section would be cool. How do you created that?

Yeah. I was planning to create that after this PR. I prefer to do it myself to ensure it's retina and the same style as the existing one. Here's how I do it: sindresorhus/ama#382

Private packages are not published to npm, that's why we don't need the "choose your dist-tag" step.
Have a look at this already existing code in index.js

Right. I was thinking of private packages, which is a thing, not "private": true in package.json.

@zinserjan
Copy link
Contributor Author

Yeah. I was planning to create that after this PR. I prefer to do it myself to ensure it's retina and the same style as the existing one.

Can you also add the desired instructions about the interactive ui when you do this? I have no clear idea about this 😁

@zinserjan
Copy link
Contributor Author

@sindresorhus please have a look again. I've made the requested changes except the instruction things.

@sindresorhus
Copy link
Owner

sindresorhus commented Oct 10, 2016

Looks great to me. I'm gonna use it for some days to ensure it's working fine and look for possible improvements.

@SamVerschueren @kevva @silverwind @scott113341 @unkillbob @developit @Hypercubed Would be awesome if you could try this out and provide some feedback if anything ;)

You can install this PR with: $ npm i -g 'zinserjan/np#interactive-ui' and run it with $ np.

Comment your npm username if you want to get added to https://www.npmjs.com/package/sindre-playground to be able to test publishing with a test package.

@sgulseth
Copy link

should perhaps be [version], since it's an optional argument.

@sgrif
Copy link

sgrif commented Oct 10, 2016

What is the difference between prepatch and prerelease?

@ragingwind
Copy link

Please add me. npm-id: ragingwind

@unkillbob
Copy link
Contributor

Tested on a couple of real publishes, worked awesome, love it 👌 💯

@sindresorhus
Copy link
Owner

What is the difference between prepatch and prerelease?

That is a very good question. I don't even know. We copied those from npm version --help. The best explanation I could find is in the semver code and it's not even clear: https://github.com/npm/node-semver/blob/d21444a0658224b152ce54965d02dbe0856afb84/semver.js#L411-L413

@sindresorhus
Copy link
Owner

Please add me. npm-id: ragingwind

Done

@SamVerschueren
Copy link
Collaborator

It turns out the Node.js check is broken

screen shot 2016-10-11 at 08 56 40

@sindresorhus
Copy link
Owner

should perhaps be [version], since it's an optional argument.

👍

@zinserjan
Copy link
Contributor Author

should perhaps be [version], since it's an optional argument.

Yep, makes sense.

What is the difference between prepatch and prerelease?

I asked myself the same question a few days ago. I figured out that the option prerelease is basically just a way to increment the last pre release number. When the current version of the package isn't a prerelease (e.g. 1.0.0) it acts like prepatch.

Some examples how each option affects the version:

# starting from a normal release version
1.0.0 => prerelease=> 1.0.1-0
1.0.0 => prepatch => 1.0.1-0
1.0.0 => preminor=> 1.1.0-0
1.0.0 => premajor=> 2.0.0-0

# starting from a pre release version
1.0.1-0 => prerelease=> 1.0.1-1
1.1.0-0 => prerelease=> 1.1.0-1
2.0.0-0 => prerelease=> 2.0.0-1

It turns out the Node.js check is broken

Good catch. The version check is in the wrong order.

@SamVerschueren
Copy link
Collaborator

Should we put the versions in a different color (e.g. 0.2.0 and 0.3.0)?

screen shot 2016-10-12 at 12 40 57

@SamVerschueren
Copy link
Collaborator

Ran this for a couple of my releases and it works like a charm. Great work @zinserjan !

@Hypercubed
Copy link
Contributor

Works great for me as well. In fact so well I wonder if it could be it's own package. version-chooser or something.

.map(line => line.split(':')[0].replace(/^\s|\s$/, ''))
.filter(line => line.toLowerCase() !== 'latest');

if (!existingPreleaseTags.length) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would explicitely check for 0

existingPreleaseTags.length === 0

choices: () => {
return execa.stdout('npm', ['dist-tag', 'ls'])
.then(stdout => {
const existingPreleaseTags = stdout.split('\n')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

call it existringPrereleaseTags

message: 'How should this pre-release version be tagged in npm?',
when: answers => !pkg.private && version.isPrereleaseVersion(answers.version) && !options.tag,
choices: () => {
return execa.stdout('npm', ['dist-tag', 'ls'])
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Return execa immediately behind the arrow function

choices: () => execa.stdout()

message: answers => {
const tag = answers.tag || options.tag;
const msg = `Will bump from ${oldVersion} to ${answers.version}${tag ? ` and tag this release in npm as ${tag}` : ''}. Continue?`;
return msg;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this would be clearer if it was written like this

const tag = answers.tag || options.tag;
const tagPart = tag ? ` and tag this release in npm as ${tag}` : '';

return `Will bump from ${oldVersion} to ${answers.version}${tagPart}. Continue?`;

@Hypercubed
Copy link
Contributor

Hi @zinserjan , sorry, this is very tangential but are you interested in publishing your work on ./lib/ui as a separate module.

@sindresorhus
Copy link
Owner

@Hypercubed That's too premature. Better to figure out what works and iterate on this here. We can explore what can be extracted as reusable at a later point.

@sindresorhus
Copy link
Owner

sindresorhus commented Oct 22, 2016

@zinserjan Any chance you could address @SamVerschueren's feedback so we could get this merged?

@zinserjan
Copy link
Contributor Author

@sindresorhus @SamVerschueren
Done. There's an lint issue with a require statements in index.js. I just disabled this for now, cause assign to a unused variable leads to another eslint error.

@Hypercubed I don't see any reason at the moment to publish this under a separate module. When you see a need for this, feel free to do this. I personally don't need this in any other projects.

@zinserjan
Copy link
Contributor Author

Should we put the versions in a different color (e.g. 0.2.0 and 0.3.0)?

@SamVerschueren Any suggestions?

@sindresorhus sindresorhus merged commit df32774 into sindresorhus:master Nov 4, 2016
@sindresorhus
Copy link
Owner

Woot. Landed! Thanks so much for working on this @zinserjan :)

superhighfive

@SamVerschueren
Copy link
Collaborator

Great job @zinserjan !

sindresorhus added a commit that referenced this pull request Nov 4, 2016
@sindresorhus
Copy link
Owner

Dogfooding ftw!

screen shot 2016-11-04 at 15 56 46

@zinserjan
Copy link
Contributor Author

Awesome! Thanks for merging ;)

@zckrs
Copy link

zckrs commented Nov 4, 2016

Just fetched and ported to internal tool.

Thanks :-)

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

9 participants