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

Rename some methods in the os module #19110

Closed
wants to merge 2 commits into from

Conversation

barosl
Copy link
Contributor

@barosl barosl commented Nov 19, 2014

This pull request feels a bit bikeshedding, but I think it is necessary.

  • os::make_absolute() -> os::abspath(): The name "make_absolute" is too generous to hold a specific meaning. At least it should contain "path". It seems that "abspath" is a common and concise name. (Also used in Python.)
  • os::change_dir() -> os::chdir(): The method names in the os module generally follow the POSIX-like convention, such as setenv, getenv, getcwd, and errno. As os::change_dir() internally uses libc::chdir() on Unix, it would be a good idea to follow the same convention.

The name "make_absolute" is too generous to hold a specific meaning. At
least it should contain "path". It seems that "abspath" is a common and
concise name.

[breaking-change]
The method names in the os module generally follow the POSIX-like
convention, such as setenv, getenv, getcwd, and errno. As
os::change_dir() internally uses libc::chdir() on Unix, it would be a
good idea to follow the same convention.

[breaking-change]
@alexcrichton
Copy link
Member

I know that @aturon has been focused on std::io recently, and it's likely that we're going to move away from the posix-based names in general (like chdir) to more descriptive names which don't bias one platform over another. For that reason, this may want to hold off on the change_dir => chdir rename for now.

Additionally, the make_absolute => abspath change seems related to rust-lang/rfcs#474 so you may want to coordinate with @aturon on that as well.

@michaelsproul
Copy link
Contributor

+1 for Alex's comment. I think internal naming consistency is more desirable than following Python/Unix convention.

@barosl
Copy link
Contributor Author

barosl commented Nov 19, 2014

I'm just a bit confused of dealing with the different conventions at the same time. If we can use the descriptive names for other methods in the os module as well, I don't care. (Such like setenv() -> set_env_var()?)

But I also think that if we are to unify the names into the descriptive fashion, the required changes should be larger than the reverse. So I think sticking to POSIX is still a viable option.

One thing to note, which is slightly (not?) related to this problem is, that the reason why make_absolute() is in os is that it depends on os::getcwd(). I'm not sure this is a valid design, because to me, it seems to be natural that the method is part of Path. Maybe we can discuss this at rust-lang/rfcs#474.

@alexcrichton
Copy link
Member

Yes there are definitely conflicting conventions throughout std::io::fs and std::os, but these are slated to be dealt with when the modules are considered for API stabilization. The POSIX naming convention is definitely a viable option, but we need to have an overall vision for the entire set of I/O apis instead of locally optimizing one module only to maybe have it change radically later.

I also agree that os::make_absolute is odd when I would expect it in the path module!

@bstrie
Copy link
Contributor

bstrie commented Nov 20, 2014

Does @aturon have a formal RFC for naming conventions like this yet?

@alexcrichton
Copy link
Member

Not that I know of, but I'm sure it's coming down the pike!

@alexcrichton
Copy link
Member

Closing due to inactivity, but as an update @aturon and I gave a close look to the std::os module this past work week and an RFC/PR should be coming soon!

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.

4 participants