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

Revert #17398 and #17402 #18480

Merged
merged 3 commits into from
Jul 18, 2021
Merged

Revert #17398 and #17402 #18480

merged 3 commits into from
Jul 18, 2021

Conversation

narimiran
Copy link
Member

Another breaking change which shouldn't be part of Nim 1.6

@juancarlospaco
Copy link
Collaborator

If someone really relies on broken behaviours and not asserting its inputs,
then that person should do a tiny "shim", instead of keeping the bad code for everyone,
is very cheap to check if a char is DirSep. 🤷

template getHomeDirShim*(): string =
  let s = getHomeDir()
  if s[^1] == '/': s else: s & '/'

@timotheecour
Copy link
Member

or, using getHomeDir().normalizePathEnd(trailingSep = false)

@kaushalmodi
Copy link
Contributor

Wouldn't it be better to create a version-1p6 branch and undo all the breaking commits there? I thought devel was targeting the latest and greatest future Nim version.

@Araq
Copy link
Member

Araq commented Jul 13, 2021

Wouldn't it be better to create a version-1p6 branch and undo all the breaking commits there? I thought devel was targeting the latest and greatest future Nim version.

We're considering it.

@timotheecour
Copy link
Member

timotheecour commented Jul 13, 2021

Wouldn't it be better to create a version-1p6 branch and undo all the breaking commits there? I thought devel was targeting the latest and greatest future Nim version.

this is would be much better than reverting these commit, likewise with other reverts that are currently pending and other similar reverts that were made in 1.5; but we don't even need to undo those commits in that branch and in fact it's better not to as it allows gradual migration, see #18486

else: result = getEnv("HOME")
result.normalizePathEnd(trailingSep = defined(nimLegacyHomeDir))
when defined(windows): return getEnv("USERPROFILE") & "\\"
else: return getEnv("HOME") & "/"
Copy link
Member

Choose a reason for hiding this comment

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

So previously the code used a trailing dir on purpose. Calling this a "bugfix" then is a bit of stretch...

@Araq
Copy link
Member

Araq commented Jul 16, 2021

then that person should do a tiny "shim", instead of keeping the bad code for everyone

And why is it suddenly "bad code"? It used to work and could continue to work...

@timotheecour
Copy link
Member

timotheecour commented Jul 17, 2021

See #18513 ({.migrated:... .}) which provides a much better solution, by issuing a new migration warning for these 3 APIs, which allows you to find at CT all the callsites that need to be checked; the migration warning can be turned off on a per-API basis (eg -d:nimMigratedGetHomeDir) which should make the migration a lot easier.

And why is it suddenly "bad code"? It used to work and could continue to work...

because it was inconsistent both with all other languages and all other APIs in nim that return a dir, see long list here: #17393 (comment), consistency is still a thing.

@Araq
Copy link
Member

Araq commented Jul 18, 2021

because it was inconsistent both with all other languages and all other APIs in nim that return a dir, see long list here: #17393 (comment), consistency is still a thing.

But now it's not consistent with its own previous behavior anymore... Great consistency here.

@Araq Araq merged commit 8091545 into nim-lang:devel Jul 18, 2021
PMunch pushed a commit to PMunch/Nim that referenced this pull request Mar 28, 2022
* Revert "followup nim-lang#17398: `getTempDir`, `getConfigDir` now do not have trailing DirSep (nim-lang#17402)"

This reverts commit 2356d06.

* Revert "fix nim-lang#17393 getHomeDir and expandTilde should not include trailing `/` (nim-lang#17398)"

This reverts commit bebf2ce.

* fix test
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.

5 participants