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 File::with_options to builder #76744

Closed
wants to merge 2 commits into from
Closed

Conversation

tesuji
Copy link
Contributor

@tesuji tesuji commented Sep 15, 2020

This patch implements this idea: #65439 (comment)

Java APIs often use builder pattern and name the builder function as builder.
At least we have a prio-art to follow. An arguments against with_options
is that it often receives arguments.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 15, 2020
@tesuji
Copy link
Contributor Author

tesuji commented Sep 15, 2020

Wow, there are some people using File::with_options: https://github.com/search?l=Rust&q=%22File%3A%3Awith_options%22&type=Code . Gonna open an stabilizing PR after this.

@tesuji
Copy link
Contributor Author

tesuji commented Sep 15, 2020

seems like Ashley is busy, gonna ping other T-libs member: r? @dtolnay

@rust-highfive rust-highfive assigned dtolnay and unassigned KodrAus Sep 15, 2020
@jyn514 jyn514 added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Sep 15, 2020
@@ -358,32 +358,26 @@ impl File {
OpenOptions::new().write(true).create(true).truncate(true).open(path.as_ref())
}

/// Returns a new OpenOptions object.
/// Returns a builder struct to configure how a file is opened.
Copy link
Contributor

@pickfire pickfire Sep 16, 2020

Choose a reason for hiding this comment

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

It is not a builder struct but OpenOptions struct.

Suggested change
/// Returns a builder struct to configure how a file is opened.
/// Configure how to open a file with `OpenOptions`.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OpenOptions struct is a builder struct. I'm not sure we want to repeatedly mention
OpenOptions. I'll leave this for reviewer to decide.

#[unstable(feature = "with_options", issue = "65439")]
pub fn with_options() -> OpenOptions {
#[unstable(feature = "file_builder", issue = "65439")]
pub fn builder() -> OpenOptions {
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder why not options?

Copy link
Contributor Author

@tesuji tesuji Sep 16, 2020

Choose a reason for hiding this comment

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

I guess I've never seen APIs like that. But yeah, it could be if reviewer prefers.

Copy link
Contributor

Choose a reason for hiding this comment

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

Based on the informal poll in #65439, builder was the least popular name and options was the most popular (just).

Copy link
Contributor Author

@tesuji tesuji Sep 16, 2020

Choose a reason for hiding this comment

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

I know that. However Rust design has never been a community vote, but
it depends much on official respective teams.
(To be clear: I don't belong to any Rust teams).

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, just pointing it out in case whoever reviews this wasn't aware.

Copy link
Contributor

Choose a reason for hiding this comment

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

Wow, I didn't even know there is a vote. I just came out with it from my mind. Good to know that we are on the same page and same taste. :D

Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

I think I would prefer to let this play out in the tracking issue a little further. As it stands, due to the lack of consensus I think a rename/stabilization here would require disproportionately much attention and for very little benefit, so I think it would be better for the team to prioritize other things.

@dtolnay dtolnay closed this Sep 19, 2020
@tesuji
Copy link
Contributor Author

tesuji commented Sep 19, 2020

let this play out in the tracking issue a little further

So the team now prefers the community votes over team designs and decisions.
That's fine by me, really. I could open a topic in URO or reddit for people to vote.

due to the lack of consensus

What are consensus you talking about? If you don't have strong opinion on it, you could reassign it to others.
We could always do a FCP for the API in stabilization issue/PR.


With all respects, I am really disappointed with how things done in this PR.
It's not because you close the PR, but you don't raise meaningful objective points about the API
and the renaming. And you just say the team doesn't have the bandwidth to
decide how API should be.

@tesuji tesuji deleted the fs_options branch September 19, 2020 06:48
@dtolnay
Copy link
Member

dtolnay commented Sep 19, 2020

To clarify, please don't hold a URO/reddit vote.

you don't raise meaningful objective points about the API and the renaming. And you just say the team doesn't have the bandwidth to decide what API should look like.

This is sadly just how bandwidth works. Expecting that a team would engage to raise and discuss meaningful objective points on anything someone cares about regardless of rate of PRs opened and priorities, is not realistic.

@tesuji
Copy link
Contributor Author

tesuji commented Sep 19, 2020

It's alright for your explanation to clarify things.
I'm not aware any issues would block this
but I agree that the bike-shred might be high for current state of lib team.
Thank you for your thoughts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants