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

Rename some files to be more cross platform #2397

Merged
merged 1 commit into from
Jan 6, 2021
Merged

Conversation

nmarghetti
Copy link
Contributor

@nmarghetti nmarghetti commented Jan 3, 2021

Issue: the repository cannot be cloned under Windows system as:

  • the characters \/:*?"<>| are not allowed in filename
  • filename cannot end with '.'

The script rename_test.sh fix those problem for the files under test folder as it:

  • replaces " by '
  • replaces <> by <> not in ASCII
  • removes . at the end of filename

@nmarghetti nmarghetti force-pushed the rename branch 3 times, most recently from 3a5735b to b7ed9a9 Compare January 3, 2021 14:26
@nmarghetti
Copy link
Contributor Author

If this PR is ok, I could update the other one for Windows to allow installation by default (from git) instead of script. I would have to update the Windows CI and the Readme.

@ljharb
Copy link
Member

ljharb commented Jan 3, 2021

This would close #1643, i think.

I don’t like having to do this - the repo shouldn’t suffer just because windows’ filesystem is dumb - but if it will help test real windows support (and if the tests will ensure filenames obey the needed conventions moving forward) then it’s probably worth it.

rename_test.sh Outdated
while read -r filename; do
new_filename=$(echo "$filename" | sed -r \
-e "s/\"/'/g" \
-e 's/</\[/g' \
Copy link
Member

Choose a reason for hiding this comment

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

I’m concerned about this one - there’s a meaningful difference here. <foo> is both convention for a replacement in a way [foo] is not, and also, in command line notation, one means a required argument and the other an optional one.

is there another character that might work that looks visually similar to < >, or are we limited to ascii?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I dont like much to have to put [] instead of <> for the meaning. I ll check if I can find something that look more similar.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess I found a solution :D
What do you think now ?

@ljharb ljharb added OS: windows testing Stuff related to testing nvm itself. labels Jan 3, 2021
@nmarghetti nmarghetti force-pushed the rename branch 5 times, most recently from 04487a7 to 2d66c6f Compare January 4, 2021 00:25
@nmarghetti
Copy link
Contributor Author

I have also added a check in the linting CI to ensure no more tests are added with those kind of filename.

@nmarghetti nmarghetti requested a review from ljharb January 4, 2021 00:32
@ljharb ljharb force-pushed the rename branch 2 times, most recently from 8dad2ee to 9849bf4 Compare January 6, 2021 22:26
@ljharb ljharb merged commit 9849bf4 into nvm-sh:master Jan 6, 2021
@TheLordDrake
Copy link

Would there be any chance of getting this in a release soon?

@ljharb
Copy link
Member

ljharb commented Mar 13, 2021

It’ll be in the next release for sure. I’m not certain when that will be.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OS: windows testing Stuff related to testing nvm itself.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants