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

fix 3 minor bugs in joinPath (see #13455) #13462

Merged
merged 4 commits into from
Feb 23, 2020
Merged

Conversation

a-mr
Copy link
Contributor

@a-mr a-mr commented Feb 21, 2020

this fixes 3 wrong cases for joinPath:

"" / "" == "/", should be ""
"/" / "" == "//", should be "/"
"/" / "/a/b/c" == "//a/b/c", should be "/a/b/c"

I don't fix behavior like "a" / "" == "a/" since it's specified so in documentation hence changing it might break compatibility, though I admit @timotheecour is right in #13455 that it's strange.

@timotheecour
Copy link
Member

timotheecour commented Feb 21, 2020

thanks! a definite improvement

I don't fix behavior like "a" / "" == "a/" since it's specified so in documentation hence changing it might break compatibility, though I admit @timotheecour is right in #13455 that it's strange.

this can be debated/fixed in subsequent PR's, however your PR introduces a new inconsistency; which I'm sure can be addressed in your PR: when head ends with / it strips /; when it doesn't end with / it adds it

echo joinPath("././foo/.//","././") # pre-existing inconsistency
echo joinPath("foo/","") # new inconsistency that wasn't there before this PR
echo joinPath("foo","")

after PR:

foo # pre-existing inconsistency
foo # new inconsistency
foo/

before PR:

foo
foo/
foo/

so how about changing the logic so that joinPath("foo/","") returns foo/, to at least avoid adding an inconsistency; and in future PR we can revisit this and consider making all of these return foo (that debate should happen somewhere else)

@a-mr
Copy link
Contributor Author

a-mr commented Feb 21, 2020

wow, well-spotted, thanks!
Inconsistency is fixed.

@Araq
Copy link
Member

Araq commented Feb 22, 2020

Needs a changelog entry and .since support.

@a-mr
Copy link
Contributor Author

a-mr commented Feb 22, 2020

I'm not sure that I understood the point of pragma .since.

@timotheecour
Copy link
Member

timotheecour commented Feb 22, 2020

@a-mr your tests are failing because you need include system/inclrtl for since, HOWEVER:

I'm not sure that I understood the point of pragma .since.

I agree with you, IMO since shouldn't be used in this particular case; the old behavior was a just bug, not something useful/justifiable; code that depends on compiler/stdlib bugs to work is bound to break. --useVersion:1.0 should be irrelevant here.

we should backport this otherwise user code will need to be sprinkled with when (NimMajor, NimMinor) >= (1,1) to support both nim 1.0.X and nim 1.1.

So I suggest to not use since in this case.

@a-mr
Copy link
Contributor Author

a-mr commented Feb 23, 2020

Thank you for the great explanation. I removed since.

@Araq Araq merged commit 3dad130 into nim-lang:devel Feb 23, 2020
sthagen added a commit to sthagen/nim-lang-Nim that referenced this pull request Feb 23, 2020
fix 3 minor bugs in joinPath (see nim-lang#13455) (nim-lang#13462) [backport]
narimiran pushed a commit that referenced this pull request Apr 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants