cli
cli copied to clipboard
npm
fix: npm-version does not git-commit nor git-tag when package in subdirectory
libnpmversion only checks one same dir if is git repo, instead use git.find to check parents as well
References
fixes #2010
@fernandopasik Can you add a test case for this behavior?
yes, I'll add the test case. I'm trying to understand why other tests have failed.
git is mocked in the tests - we don't use the package there.
'@npmcli/git': gitMock
@fernandopasik I opened a PR into yours with what I hope are the right changes for the tests. I'm a GitHub noob, so this may have been a dumb way of doing it, but you should be able to see the ideas. If this stays idle for a few days, I can open up my own MR into this repo with both our changes.
How do we get the tests re-run? It looks like they only run once on PR open, but not on subsequent pushes, at least automatically. Are we waiting on a manual action by a maintainer?
I see failed tests from yesterday - it's not obvious to me why they're failing. I cloned the repo and can try and debug them, but I may not get to it until next week.
Just needed a good night's sleep. https://github.com/fernandopasik/cli/pull/2 fixes the tests.
I'm curious on how this will interact with workspaces, I believe it's important to make sure this change is not going to enable every npm version -w <ws-name> to commit/tag, at least not until https://github.com/npm/rfcs/issues/570 is ready
According to this the design intent is to omit tagging when versioning workspaces. We can still create commits. I'll attempt to add a new condition before tagging that checks if we're in a workspace. This will require good tests, too.
Having gotten more familiar with this, I'm leaning toward detecting workspaces, and then doing no tag and no commit. The convention we use in --git-tag-version false is "no tag, no commit". Inventing a new halfway commit with no tag for this use case doesn't seem wise.
I think I've got the workspace case covered now. https://github.com/fernandopasik/cli/pull/3 As I say in that PR, I'm not that skilled in Node, so I may have missed something silly/obvious to a seasoned dev.
Something I thought about, but didn't implement - should --f always result in a commit, even in the workspaces case? It seems like the universal "let you do otherwise unwise things" but I worry about how overloaded it could get, turning into a footgun. I'd love to get feedback on this if it's desirable.
@timothybonci almost all the work here is yours, please feel free to open a new PR with all the changes so I don't delay you. If you do link it here and I will close my PR
Thanks for starting this @fernandopasik. I've opened https://github.com/npm/cli/pull/5442 as a fresh start with my proposed changes.