Version prefix should be a config param rather than a magic string #8014
Conversation
|
This is a call for @isaacs, as I have no opinion either way, and he might know something about the rationale for the |
|
Either way it shouldn't really matter, since the implementation I've suggested preserves the "v" by default. It just can be turned off with a configuration setting, now. :) |
|
There might be a good reason to make it non-configurable, which is why I'm asking @isaacs to weigh in. Also, adding new configuration does need to clear a usefulness bar, because npm has a confusingly large amount of it already. |
|
Also, this is going to need some tests and documentation before it will pass CI, much less be in a mergeable state. |
|
I'll work on adding those, today. What possible reason could there be to make it non-configurable, though? Regardless of whether there is a confusingly large amount of configuration, I have an active use for this change immediately, and I doubt I'm the only one whose business practices require our versions to lack the preceding "v". Obviously if you're going to check it into npmjs.org, you need the "v", but it shouldn't be required for all packages ever; especially not private repositories. |
|
Tests and documentation added. Should pass ci, now. |
| * Type: string | ||
|
|
||
| If set, alters the prefix used when tagging a new version when performing a | ||
| version increment using `npm-version`. |
There was a problem hiding this comment.
This needs a note saying that you should only use this property if you absolutely need to, due to the hazards of it breaking something completely unrelated elsewhere.
For starters, a previous version of semver.org standard:
This a section was removed in v2.0.0 (probably because it's overly prescriptive), but this is the sort of thing I was looking for – semver is a somewhat odd mixture of specification and conventions, and it's often hard to tell what's going to fall afoul of it. @isaacs doesn't have strong opposition to this (I asked him about it IRL), and since you need it, we can land it, with some reservations:
As such, I've requested that the documentation make clear that is a feature that should only be used when absolutely required, and preferably only for packages that are not going to be shared out into the wider npm ecosystem.
Every bit of conditional logic you add to a product requires support and maintenance. It's nearly impossible for those of us working on the npm CLI to remember all of the stuff it can do, and as such we give people incorrect answers when supporting them, and sometimes inadvertently break features when landing "small" changes like this because it's so difficult to get adequate test coverage over such a complicated CLI tool. (See also: this issue tracker.) Thanks for your patience, and I hope I'm not being too authoritarian, but experience has taught me that it's exactly seemingly simple changes like this that end up causing major, frequently unexpected, complications later on. |
|
I will make the changes you've requested later today. Also, I really appreciate you taking the time to explain why you are hesitant to add this feature. You are not coming across as authoritarian; only as a concerned admin. In a similar vein, I hope my questions aren't coming across as too challenging of your authority! I am only trying to understand your perspective so I can try and find the best way to approach this patch. Whatever the case, I will adjust my pull request shortly to accommodate the changes you have requested. Do you prefer concise pull requests (e.g. should I use rebase to keep it to the 3 commits I already have), or do you prefer a fully preserved history (e.g. should I just add an additional commit for these new changes)? |
|
You know, before I commit this, would it be preferable to have this as a property of the package.json instead of the npm configuration? Then it could be configured per package, and potentially available to npmjs.org and other resources, rather than hidden away. I'm not sure HOW to put it in the package.json, but I can certainly read the code and figure it out if that would be preferred. |
I generally rebase, reword commit messages, and squash before merge anyway, so it's completely up to you.
Config in |
|
And thus I learn something new in the process of learning something new. Thanks for the tip about per-project |
|
Changes submitted per your request. Should pass CI. |
|
I am not entirely sure why this build failed on Travis. It doesn't look like it failed due to my changes. |
|
The tests are unfortunately somewhat flappy on Node 0.8, probably due to something in |
|
👍 I also don't use the |
|
Merged, minor tweaks made to the docs, and landed as 2114862. Thanks for your patience, and thanks for this work! |
PR-URL: #1532 Notable Changes: * crypto: significantly reduced memory usage for TLS (Fedor Indutny & Сковорода Никита Андреевич) #1529 * net: socket.connect() now accepts a 'lookup' option for a custom DNS resolution mechanism, defaults to dns.lookup() (Evan Lucas) #1505 * npm: Upgrade npm to 2.9.0. See the v2.8.4 and v2.9.0 release notes for details. Notable items: - Add support for default author field to make npm init -y work without user-input (@othiym23) npm/npm/d8eee6cf9d - Include local modules in npm outdated and npm update (@ArnaudRinquin) npm/npm#7426 - The prefix used before the version number on npm version is now configurable via tag-version-prefix (@kkragenbrink) npm/npm#8014 * os: os.tmpdir() is now cross-platform consistent and will no longer returns a path with a trailling slash on any platform (Christian Tellnes) #747 * process: - process.nextTick() performance has been improved by between 2-42% across the benchmark suite, notable because this is heavily used across core (Brian White) #1548 - New process.geteuid(), process.seteuid(id), process.getegid() and process.setegid(id) methods allow you to get and set effective UID and GID of the process (Evan Lucas) #1536 * repl: - REPL history can be persisted across sessions if the NODE_REPL_HISTORY_FILE environment variable is set to a user accessible file, NODE_REPL_HISTORY_SIZE can set the maximum history size and defaults to 1000 (Chris Dickinson) #1513 - The REPL can be placed in to one of three modes using the NODE_REPL_MODE environment variable: sloppy, strict or magic (default); the new magic mode will automatically run "strict mode only" statements in strict mode (Chris Dickinson) #1513 * smalloc: the 'smalloc' module has been deprecated due to changes coming in V8 4.4 that will render it unusable * util: add Promise, Map and Set inspection support (Christopher Monsanto) #1471 * V8: upgrade to 4.2.77.18, see the ChangeLog for full details. Notable items: - Classes have moved out of staging; the class keyword is now usable in strict mode without flags - Object literal enhancements have moved out of staging; shorthand method and property syntax is now usable ({ method() { }, property }) - Rest parameters (function(...args) {}) are implemented in staging behind the --harmony-rest-parameters flag - Computed property names ({['foo'+'bar']:'bam'}) are implemented in staging behind the --harmony-computed-property-names flag - Unicode escapes ('\u{xxxx}') are implemented in staging behind the --harmony_unicode flag and the --harmony_unicode_regexps flag for use in regular expressions * Windows: - Random process termination on Windows fixed (Fedor Indutny) #1512 / #1563 - The delay-load hook introduced to fix issues with process naming (iojs.exe / node.exe) has been made opt-out for native add-ons. Native add-ons should include 'win_delay_load_hook': 'false' in their binding.gyp to disable this feature if they experience problems . (Bert Belder) #1433 * Governance: - Rod Vagg (@rvagg) was added to the Technical Committee (TC) - Jeremiah Senkpiel (@Fishrock123) was added to the Technical Committee (TC)
My workplace does not use the "v" prefix for our versioning, which means I cannot use npm version. While this is ultimately a mere inconvenience, it'd be nice if npm was not opinionated on this subject.