Added in comments in .nvmrc #2288

Yash-Singh1 · 2020-08-20T20:31:48Z

Overview

I added in support for comments in the .nvmrc file.

List of Updates

Modified the nvm_rc_version to strip comments
Removed support for spaces and #'s inside nvm aliases

ljharb

Let's please separate out the changes that are related to "nvmrc comments", and the rest.

ljharb · 2020-08-20T20:56:34Z

README.md

@@ -446,6 +454,28 @@ Found '/path/to/project/.nvmrc' with version <5.9>
 Now using node v5.9.1 (npm v3.7.3)
 ```

+`.nvmrc` files may have comments. The following is the notation to have a comment inside a `.nvmrc` file:


This is a pretty major feature that I'm pretty convinced I do not want. In the future it might be best to discuss this sort of thing via issue first, so you don't end up doing wasted work.

README.md

ljharb · 2020-08-20T21:36:15Z

I think before deciding what code changes are needed, to this PR or nvm itself, we should have nonzero discussion about what the goal is, and why this feature is needed.

Yash-Singh1 · 2020-08-21T03:16:10Z

nvm.sh

-  NVM_RC_VERSION="$(command head -n 1 "${NVMRC_PATH}" | command tr -d '\r')" || command printf ''
+  NVM_RC_VERSION="$(command cat "${NVMRC_PATH}" | command tr -d '\r' | sed -e 's/\s*#.*$//' | sed -e '/^\s*$/d' | command tr -d '\n' | command tr -d '\t' | command head -n 1 | sed -e 's/[[:space:]]//g')" || command printf ''


Recently, I had a .nvmrc file inside one of my projects. As you know, most projects have only one, and I wanted to add in some comments. These line changes above, are the main part of this pull request. Instead of running head on the file, I replaced it for cat. Next, the next two sed commands remove comments. I also removed newlines and tabs inside. After that, I ran head -n 1 on everything to get only the first line. In the case of the following scenario:

node # This is the nvm version

There is going to be a space thereafter where it says node. Next, I also removed spaces for that scenario. That is a quick read of what is happening in the main part of the pull request.

right, but why did you want to add comments? The file is named "nvm rc", who needs the extra hint that it's for nvm?

I understand your point. The comment does not have to be:

node # This is for nvm

It can also be:

v14.0.23 # This is the node version to be installed

As you can see, the comment gives some description to someone who didn’t come across .nvmrc. I wanted to add comments to give a quick explanation of the .nvmrc. The answer is the same as asking, why do you ever write comments. To make sure that anyone else understands your code.

Right - but comments in code should never explain "what", only "why" or history/motivation, and in this case, git log .nvmrc should tell you precisely that in the commit messages.

It doesn't hurt to add in support for comments. It gives people the choice to describe why the .nvmrc is there and what is in it. I still couldn't understand why you don't want comments in .nvmrc.

@ljharb

Say we are making a change that also allows the npm version. Then, if the .nvmrc allows comments, it will look like this:

# This is a comment node # This is the nodejs version v6.14.7 # This is the npm version

In that scenario, if we remove the head -n 1 at the end, the output will be:

node v6.14.7

In the code, we can simply pick whether we want the first version or the second version (The node version or the npm version). I don't see how it complicates everything.

@ljharb those are good puritan points, and perhaps you're right, but I think in our case on a large project, where people don't even necessarily know what nvm is let alone an nvmrc it can be helpful to explain e.g. "# we're fixing to 16.2.0 because of this bug " and or simply re-document the default behaviours "this version will be used by default when you run nvm install. It's an agreed upon, tested & supported node version for the project." Coordinating communications on projects with people with a wide range of backgrounds/seniority can mean that documenting "what" is helpful.

Isn’t that what git is for?

I agree, but I don't think expecting my interns to read a git lens comment is as easy as having them read the file contents.

Should we just put all our efforts into teaching interns the correct methodologies and stop perpetuating incompetence in our industry? Probably, yes.

On the other hand, if the world is covered in unnecessary thorns, is it easier to wear shoes than of rid the world of thorns? Sometimes the pragmatic option is the correct option.

Anyway - you make good points, and I can appreciate why you'd say no. JSON doesn't have comments either.

@ljharb I can maybe put good example - in comment there could be link to issue that require to keep node in specific version due to some complications something like - for now we have this comment on CI workflows with node version - i would like to move it into nvmrc without loosing information why it is locked to particular version.

v16.10.0 # https://github.com/facebook/jest/issues/11956

But as keeping nvm simple is also good valid point.

sladyn98 · 2021-04-16T15:46:20Z

@ljharb Do we want to take action on this one with the intention of getting it merged ?

ljharb · 2021-04-16T16:55:27Z

@sladyn98 I'm still not convinced it's worth the complexity.

sladyn98 · 2021-04-19T17:03:06Z

@ljharb if you plan to not merge in the future don't you think we should close it

ljharb · 2021-04-19T17:06:51Z

I'm undecided, which is why it's left open. I don't believe in closing a PR unless it's certain to never land.

penx · 2021-04-28T13:59:19Z

Would be very useful for "make sure you update the <docker container/CI config/package.json engines> too!" style comments

Yash-Singh1 · 2021-04-28T16:03:13Z

Do you mean allowing a .nvmrc, like this?:

“some comment”
node

The reason that this shouldn’t be added is because it removes support for aliases with double quotes and anyways, the following works:

# some comment
node

penx · 2021-04-28T16:25:11Z

No, I mean the feature (as you propose it) would be useful for adding comments to the .nvmrc to remind people to change values elsewhere.

e.g.

# this should be kept in sync with the <docker container/CI config/package.json engines/readme/wiki>
14

This was partially in response to

why this feature is needed

further up.

ljharb · 2021-04-28T17:54:44Z

@penx isn't a CI job that fails when those aren't in sync much more effective than a comment?

penx · 2021-04-28T18:18:31Z

@ljharb yes it would, but it may not always be feasible or convenient

e.g.

the place to keep in sync is not within the repository, such as a wiki or external repository
the format to keep in sync is hard to parse e.g. numbers contained in paragraphs in a markdown readme where there may be multiple mentions and the wording could change ("prerequisites: node >= 12 and <= 14.*, npm 7")
the contents of .nvmrc doesn't exactly match what you're trying to sync with (e.g. semver, ranges)
the management of the CI pipelines are done by a different team/company and it is not trivial to add new ones or modify existing ones

jasonkarns · 2022-02-07T18:29:46Z

Another useful example of a useful comment (and one that isn't particularly suited for git commit messages, IMO):

14.0.0

# symlinked as .nvmrc/.node-version to support nvm, nodenv and asdf with a single file
# until such time as https://github.com/nodejs/version-management/issues/13 is resolved

ljharb · 2022-02-07T19:33:01Z

@jasonkarns i mean, the symlink part is already self-evident from "the actual symlinks in the filesystem"; it's just the issue URL that has no place to go. but fair, that's a good use case.

jasonkarns · 2022-02-09T05:35:57Z

@ljharb if we assume that it being a symlink is even evident at all.

Some ides/editors don't distinguish that a file is a symlink; and even those that do, don't necessarily make it -obvious-.

Add insult to injury, these are hidden files so many project views wouldn't show them in a listing.

All that to say: I -had- this exact comment in a symlink in a project recently and had to remove it for compatibility. :)

JohnAlbin · 2022-05-02T16:12:29Z

First, about why you want comments in the .nvmrc file:

git log is horrible for file type discovery. git log .nvmrc is going to show "Initial commit" for 100% of my projects.

If you are explaining what an .nvmrc file is for, wouldn't that be better put in a docs/DEVELOPMENT.md file? Your project readme files should explain what tools and commands are needed for development and how they are used. Having new developers poke around in all the files and to try to figure out what is important and why is not a great onboarding strategy. However, having an intern make notes about project files (even if the notes are "???") and then tasking them with expanding a project's developer docs is a GREAT intern task, @danielrob.

Several people have pointed this out above, so I'd like to reiterate the main reason I have comments in configuration files is to document bugs that force me to use a specific configuration option, .e.g. "Bug X causes Y so we set our config to Z as a work-around. [link to issue]" That kind of information needs to be right next to the configuration or someone (maybe even me!) will "fix it" later not realizing it causes a bug, e.g. "Huh, this file is out-of-date. The value of v14.13.1 should be updated to the latest v14.19.1. Oops. I broke something."

IMO, I think all configuration files should be able to have comments in them. Don't get me started on .json files for configuration.

Lastly, I was looking at the code this PR changes and noticed this line in the original nvm.sh file:

NVM_RC_VERSION="$(command head -n 1 "${NVMRC_PATH}" | command tr -d '\r')" || command printf ''

Only the first line in .nvmrc is ever used to determine the value of NVM_RC_VERSION. That means all the other lines in .nvmrc can be used for comments without any code changes. I tested this by creating a .nvmrc file with an intentionally blank first line:

v14

And I got an error: Warning: empty .nvmrc file found

And when I changed the file to:

v14
# This is a comment
// And so is this
; And so is this
And so is this

The nvm use command reported Found '.nvmrc' with version <14> and ignored all other lines except the first.

So, all we really need to do is make this officially supported in docs; no code changes, afaics.

ljharb · 2022-05-02T18:13:59Z

@JohnAlbin however, if you do that, it's guaranteed to break in the future, because eventually nvm will support parsing additional content from successive lines.

JohnAlbin · 2022-05-09T06:45:16Z

@ljharb said:

if you do that, it's guaranteed to break in the future, because eventually nvm will support parsing additional content from successive lines.

Agreed.

From my previous comment:

…all we really need to do is make this officially supported in docs… (emphasis added)

Which is why I said we need to decide how to document how comments should work.

Right now, afaics, the entire .nvmrc syntax specification is:

The first line must contain a Node.js version number; see https://github.com/nvm-sh/nvm#nvmrc
Other lines are currently ignored but are reserved for future usage.

To add the official ability to comment the file we need to decide what format comments are allowed to use. And whether we want to reserve the second or third lines for forward compatibility with future features.

FYI, I prefer the # comment syntax as it matches most of the other dot file comment syntaxes.

r-moore · 2022-07-26T08:31:39Z

Several people have pointed this out above, so I'd like to reiterate the main reason I have comments in configuration files is to document bugs that force me to use a specific configuration option, .e.g. "Bug X causes Y so we set our config to Z as a work-around. [link to issue]" That kind of information needs to be right next to the configuration or someone (maybe even me!) will "fix it" later not realizing it causes a bug, e.g. "Huh, this file is out-of-date. The value of v14.13.1 should be updated to the latest v14.19.1. Oops. I broke something."

This is our exact use case

We pin a project to a specific node version and I would like to leave a comment in nvmrc to say why we are using that version (e.g. "don't upgrade to v16 yet because X breaks")

Yes we could put that comment somewhere else in documentation, but this seems like the best place to leave a reminder (so that the person bumping the version will see it)

JohnAlbin · 2024-04-18T07:07:35Z

I created a new issue so we can discuss adding comments to .nvmrc without mixing that discussion up with our opinions about the code in this PR.

#3336 Allow comments in .nvmrc config file

ljharb requested changes Aug 20, 2020

View reviewed changes

ljharb added the feature requests I want a new feature in nvm! label Aug 20, 2020

This comment has been minimized.

Sign in to view

Yash-Singh1 commented Aug 21, 2020

View reviewed changes

ljharb force-pushed the master branch from 4556832 to 978a9f8 Compare August 24, 2020 06:26

ljharb force-pushed the master branch from a09e064 to 49e806c Compare February 1, 2021 22:36

ljharb force-pushed the master branch from d3710f8 to 07253ec Compare April 16, 2021 05:45

ljharb marked this pull request as draft April 16, 2021 16:55

github-actions bot force-pushed the master branch from 1d6999d to d7ba60d Compare April 18, 2021 22:26

Yash-Singh1 force-pushed the master branch from 7ae00fe to eb338c8 Compare April 19, 2021 17:53

Yash-Singh1 changed the title ~~Added in comments in .nvmrc and more~~ Added in comments in .nvmrc Apr 19, 2021

Yash-Singh1 force-pushed the master branch from e9d89ef to 637ad8c Compare April 20, 2021 16:52

Yash-Singh1 force-pushed the master branch from 234e055 to 4a150a1 Compare May 6, 2021 20:21

Yash-Singh1 added 7 commits May 11, 2021 15:31

Added in comments in .nvmrc and more

8cd1fd2

Updates

e1bbf0b

Possibly fix a few tests

c891425

Fix unbound variable

fb37285

Fix filename

f6bca73

Fix linter

33c74fe

Fix eclint

de8ac9c

Remove extra help

a680f38

Yash-Singh1 force-pushed the master branch from 99fd1c9 to a680f38 Compare May 11, 2021 22:32

Yash-Singh1 added 2 commits November 9, 2021 11:44

Merge branch 'nvm-sh:master' into master

f7bed10

Merge branch 'master' into master

c06050d

ljharb force-pushed the master branch from 7846e33 to 1f970cc Compare November 1, 2023 22:11

JohnAlbin mentioned this pull request Apr 18, 2024

Allow comments in .nvmrc config file #3336

Closed

ljharb closed this in 29dce5e Jun 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added in comments in .nvmrc #2288

Added in comments in .nvmrc #2288

Yash-Singh1 commented Aug 20, 2020 •

edited

Loading

ljharb left a comment

ljharb Aug 20, 2020

ljharb commented Aug 20, 2020

This comment has been minimized.

Yash-Singh1 Aug 21, 2020

ljharb Aug 21, 2020

Yash-Singh1 Aug 21, 2020

ljharb Aug 21, 2020

Yash-Singh1 Aug 21, 2020

Yash-Singh1 Aug 21, 2020 •

edited

Loading

danielrob Nov 29, 2021

ljharb Nov 29, 2021

danielrob Nov 29, 2021

bambuchaAdm Sep 12, 2023

sladyn98 commented Apr 16, 2021

ljharb commented Apr 16, 2021

sladyn98 commented Apr 19, 2021

ljharb commented Apr 19, 2021

penx commented Apr 28, 2021 •

edited

Loading

Yash-Singh1 commented Apr 28, 2021

penx commented Apr 28, 2021 •

edited

Loading

ljharb commented Apr 28, 2021

penx commented Apr 28, 2021 •

edited

Loading

jasonkarns commented Feb 7, 2022

ljharb commented Feb 7, 2022

jasonkarns commented Feb 9, 2022

JohnAlbin commented May 2, 2022 •

edited

Loading

ljharb commented May 2, 2022

JohnAlbin commented May 9, 2022

r-moore commented Jul 26, 2022

JohnAlbin commented Apr 18, 2024

		NVM_RC_VERSION="$(command head -n 1 "${NVMRC_PATH}" \| command tr -d '\r')" \|\| command printf ''
		NVM_RC_VERSION="$(command cat "${NVMRC_PATH}" \| command tr -d '\r' \| sed -e 's/\s#.$//' \| sed -e '/^\s*$/d' \| command tr -d '\n' \| command tr -d '\t' \| command head -n 1 \| sed -e 's/[[:space:]]//g')" \|\| command printf ''

Added in comments in .nvmrc #2288

Added in comments in .nvmrc #2288

Conversation

Yash-Singh1 commented Aug 20, 2020 • edited Loading

Overview

List of Updates

ljharb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljharb commented Aug 20, 2020

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yash-Singh1 Aug 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sladyn98 commented Apr 16, 2021

ljharb commented Apr 16, 2021

sladyn98 commented Apr 19, 2021

ljharb commented Apr 19, 2021

penx commented Apr 28, 2021 • edited Loading

Yash-Singh1 commented Apr 28, 2021

penx commented Apr 28, 2021 • edited Loading

ljharb commented Apr 28, 2021

penx commented Apr 28, 2021 • edited Loading

jasonkarns commented Feb 7, 2022

ljharb commented Feb 7, 2022

jasonkarns commented Feb 9, 2022

JohnAlbin commented May 2, 2022 • edited Loading

ljharb commented May 2, 2022

JohnAlbin commented May 9, 2022

r-moore commented Jul 26, 2022

JohnAlbin commented Apr 18, 2024

Yash-Singh1 commented Aug 20, 2020 •

edited

Loading

Yash-Singh1 Aug 21, 2020 •

edited

Loading

penx commented Apr 28, 2021 •

edited

Loading

penx commented Apr 28, 2021 •

edited

Loading

penx commented Apr 28, 2021 •

edited

Loading

JohnAlbin commented May 2, 2022 •

edited

Loading