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

exporting normalizePathEnd #13000

Closed
wants to merge 3 commits into from
Closed

Conversation

disruptek
Copy link
Contributor

I dunno if this is the best way to conditionally export this proc in 1.1.

@timotheecour
Copy link
Member

I dunno if this is the best way to conditionally export this proc in 1.1.

seems like the best way to me

@disruptek
Copy link
Contributor Author

What do you think about renaming the copy version to normalizedPathEnd?

@timotheecour
Copy link
Member

timotheecour commented Jan 1, 2020

my 2 cents: I know there's a precedent for that (eg: sort vs sorted) but:

but ok with whatever you decide :)

@disruptek
Copy link
Contributor Author

Divergent symbols are easier to recognize in IDE completion and easier to control in import statements, but if no one wants to rename it, I'm fine with it as is.

@Araq
Copy link
Member

Araq commented Jan 2, 2020

The problem with this PR is that technically speaking you're relying on a compiler bug to achieve this. export is for exporting symbols from an imported module.

@timotheecour
Copy link
Member

The problem with this PR is that technically speaking you're relying on a compiler bug to achieve this. export is for exporting symbols from an imported module.

the "compiler bug" is IMO the more useful, consistent, less surprising behavior (and I've written code relying on that); we can just update the spec accordingly. The use-case in this PR is a good example; if we followed current spec, it'd lead to less DRY code.

@disruptek
Copy link
Contributor Author

Hence my first comment. How would you like to do it?

@Araq
Copy link
Member

Araq commented Jan 2, 2020

the "compiler bug" is IMO the more useful, consistent, less surprising behavior (and I've written code relying on that); we can just update the spec accordingly.

Agreed, however it then needs to have tests of its own to ensure we don't break this feature. And this also includes little things like "nim doc needs to list every exported proc..."

@timotheecour
Copy link
Member

And this also includes little things like "nim doc needs to list every exported proc..."

proc main()=discard
export main

just checked, nim doc indeed needs to be fixed to list main as public symbol instead of adding main in exports section. hopefully not hard


else:
proc normalizePathEnd(path: var string, trailingSep = false) =
## Ensures ``path`` has exactly 0 or 1 trailing `DirSep`, depending on
Copy link
Member

Choose a reason for hiding this comment

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

i really don't like code duplication and we don't need runnableExamples + unittest for this un-exported version, all this is already in normalizePathEnd*

@timotheecour
Copy link
Member

let's wait until #13123 is merged, then the simple:

when (NimMajor, NimMinor) >= (1, 1):
  export normalizePathEnd

would work, avoiding all the above issues

@timotheecour
Copy link
Member

timotheecour commented Jan 13, 2020

@disruptek #13123 got merged so you can now rebase against latest devel and use the simple

when (NimMajor, NimMinor) >= (1, 1):
  export normalizePathEnd

@Araq
Copy link
Member

Araq commented Jan 14, 2020

@timotheecour It still needs to be documented.

@timotheecour
Copy link
Member

@timotheecour It still needs to be documented.

this is what I mean: #13152 ; it shows documentation correctly and is simpler than this PR (and DRY)

@disruptek
Copy link
Contributor Author

Nice.

@disruptek disruptek closed this Jan 15, 2020
@disruptek disruptek deleted the normalizepathend branch September 12, 2020 22:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge_when_passes_CI mergeable once green
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants