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

proposal: os: add PathSeparatorString and PathListSeparatorString #30614

Open
ianlancetaylor opened this Issue Mar 6, 2019 · 4 comments

Comments

Projects
None yet
4 participants
@ianlancetaylor
Copy link
Contributor

commented Mar 6, 2019

Issue #3939 proposes eliminating the conversion from integer types to string types. Examining code shows that one common case where the conversion is used correctly is string(os.PathSeparator). If we eliminate the conversion from the language, we will need a replacement for code of that sort, ideally one that does not involve a function call.

I propose that we add two new constants to the os package: PathSeparatorString and PathListSeparatorString. These are the best names I've come up with, but they are not great, and I would be happy to hear better suggestions. The new constants will have the same value as the current constants PathSeparator and PathListSeparator, but will be untyped string constants rather than untyped rune constants.

Similarly, I propose that we add SeparatorString and ListSeparatorString to the path/filepath package.

@ianlancetaylor ianlancetaylor added this to the Proposal milestone Mar 6, 2019

@networkimprov

This comment has been minimized.

Copy link

commented Mar 6, 2019

To my eye "delimit" is a better term than "separate" re paths.

Although PathDelimiterString isn't shorter, PathDelimiter and PathDelimString are.

I'll be sad to see string(rune(n)) disappear. That would break lots of things. Will it be go vet flagged in 1.13?

@cuonglm

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2019

How about put the same variables but under strings package:

const (
        PathSeparator     = string(os.PathSeparator)
        PathListSeparator = string(os.PathListSeparator)
)

So caller can use:

strings.PathSeparator
strings.PathListSeparator

to get string version of them.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2019

@networkimprov I don't see a good reason to change from Separator to Delimiter. We have existing functions like os.IsPathSeparator and no good reason to change them.

If removing string(rune(n)) will break lots of things, please give examples on #3939. It may be that we should not accept that proposal.

@Gnouc Separators are logically part of the os and path/filepath packages. They don't have anything to do with the strings package.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2019

Whether this is necessary depends on the details of #3939. Let's put this on hold for now.

@rsc rsc added the Proposal-Hold label Mar 6, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.