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

Jump to bottom

proposal: os: add PathSeparatorString and PathListSeparatorString #30614

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

proposal: os: add PathSeparatorString and PathListSeparatorString #30614

ianlancetaylor opened this issue Mar 6, 2019 · 4 comments
Labels
Proposal Proposal-Hold
Milestone
Proposal

Comments

@ianlancetaylor
Copy link
Contributor

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
Copy link

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
Copy link
Member

cuonglm 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
Copy link
Contributor Author

@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 rsc mentioned this issue Mar 6, 2019
Open
@rsc
Copy link
Contributor

rsc commented Mar 6, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Proposal Proposal-Hold
Projects
None yet
Milestone
Proposal
Development

No branches or pull requests
4 participants
@rsc @networkimprov @ianlancetaylor @cuonglm