Np: `np` doesn't actually check for unpulled changes (before showing version selection in interactive mode)
Description
The readme states:
Ensures the working directory is clean and that there are no unpulled changes
When trying np for the first time on a random repo I had checked out, this did not happen though:
E:\Projects\Cordova\cordova-plugin-splashscreen (master -> origin) ([email protected])
λ np
Publish a new version of cordova-plugin-splashscreen (current: 5.0.3-dev)
Commits:
- CI: Use universal paramedic travis.yml (#210) 6800de2
...
- Fixed docs typo 5f41762
Commit Range:
5.0.2...master
? Select semver increment or specify new version (Use arrow keys)
> patch 5.0.3
minor 5.1.0
major 6.0.0
prepatch 5.0.4-0
preminor 5.1.0-0
premajor 6.0.0-0
prerelease 5.0.3-dev.0
──────────────
Other (specify)
As
E:\Projects\Cordova\cordova-plugin-splashscreen (master -> origin) ([email protected])
λ git status
On branch master
Your branch is behind 'origin/master' by 4 commits, and can be fast-forwarded.
(use "git pull" to update your local branch)
nothing to commit, working tree clean
(4 unpulled changes) both the current version 5.0.3-dev and the last tag 5.0.2 used to get the commits were wrong.
Running git pull and git fetch --tags manually fixed the problem.
Expected behavior
I expected np to also check for unpulled changes (and unfetched tags possibly) and exiting before offering the version selection with wrong information to me.
Environment
np - 5.0.2
Node.js - 10.15.1
npm - 6.4.1
Git - 2.21.0.windows.1
OS - Windows 10
janpio
All 10 comments
np will only check for unpulled changes _after_ you select the version you'd like to publish, as part of the prerequisite tasks. Can you wait for those to run and see if you then get an error reported about the unpulled commits?
If you still don't get an error reported, try running git rev-list --count --left-only @{u}...HEAD, and comment the output printed by the command.
itaisteinherz
on 23 May 2019
I now understand how it works (and indeed, selecting a version then runs into the failing check), but it wasn't what I was expected. This false start of showing a wrong version and using a wrong tag to display the commits could be avoided if the "is it up to date" check would be done before the version selection is displayed.
janpio
on 23 May 2019
This isn't really a bug, but I agree that currently np's increment selection UI is a bit confusing in this case.
Since we shouldn't remove the unpulled changes check from the prerequisite tasks (as that needs to run even if the user has provided the next version to publish), I think that there are two ways in which we could solve this (though feel free to suggest anything else you come up with):
- Check if there are any unpulled commits before printing the commit history, and if there are some print an error and exit.
- While printing the commit history, in case there are any unpulled commits, print a warning/error next to the history saying that there are currently unpulled commits
npcan't account for, and so the user should pull those before continuing.
However both of these solutions feel a bit hacky to me.
itaisteinherz
on 23 May 2019
I know almost nothing about how np works internally, but as from how you describe it the unpulled changes check _always_ has to pass before publishing (correct?), I think 1) is the better choice here. It allows reusing the same code and error message here without creating (too much) additional logic.
And yep, having the same check run twice in interactive mode feels a bit hacky. But I don't really have an alternative, other than maybe moving _all_ the checks before the increment selection UI - but that probably doesn't make sense because of other reasons (like some of the checks actually requiring the target version to work maybe?).
(While we are at it thinking about this: Are there any other checks that might be useful before showing the increment selection UI?)
janpio
on 23 May 2019
the
unpulled changescheck _always_ has to pass before publishing (correct?)
Yes
maybe moving _all_ the checks before the increment selection UI - but that probably doesn't make sense because of other reasons (like some of the checks actually requiring the target version to work maybe?).
Yeah, that's not what we're aiming for (and also the Check for pre-release version check requires the published version).
(While we are at it thinking about this: Are there any other checks that might be useful before showing the increment selection UI?)
Not that I can think of.
itaisteinherz
on 23 May 2019
In case you're interested, feel free to create a PR with what we discussed above and we'll proceed to getting this landed in np ✨
itaisteinherz
on 23 May 2019
On it.
janpio
on 23 May 2019
For anyone that wants to work on this, see the feedback in https://github.com/sindresorhus/np/pull/415.
sindresorhus
on 21 Jul 2019
Explicit links to stuff I could not take care of, which caused the PR to stall:
- missing tests: https://github.com/sindresorhus/np/pull/415#issuecomment-495252562
- another check requested: https://github.com/sindresorhus/np/pull/415#issuecomment-499381380
- remove the check from somewhere else: https://github.com/sindresorhus/np/pull/415#issuecomment-499397193
<3
janpio
on 21 Jul 2019
For anyone trying to pick this up, read through the comments in #562
I'll give a quick summary of what was decided in #562 .
- The precheck should not slow down the UI too much as per this comment . Tasks like
check remote historycould slow down the UI considerably because it involves a network call - We can still go forward and move the local checks like
verifyWorkingTreeIsClean()orverifyRemoteHistoryIsClean()to run before UI is displayed, but ideally it should run under the hood, and only show an error in case its thrown. - The rest of the implementations can continue after #570 is implemented.
dopecodez
on 18 Oct 2020
Related issues
nickthesing
·
5Comments
gillesdemey
·
3Comments
inker
·
4Comments
bennycode
·
4Comments
xxczaki
·
6Comments
Most helpful comment
Explicit links to stuff I could not take care of, which caused the PR to stall:
<3