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.

Sign up for GitHub

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

Implement AsRef<Path> for Component #46985

Merged
merged 1 commit into from
Jan 12, 2018
Merged

Implement AsRef<Path> for Component #46985

merged 1 commit into from
Jan 12, 2018

Conversation

Diggsey
Copy link
Contributor

@Diggsey Diggsey commented Dec 24, 2017

Fixes #41866

@rust-highfive
Copy link
Collaborator

r? @BurntSushi

(rust_highfive has picked a reviewer for you, use r? to override)

@kennytm kennytm added relnotes Marks issues that should be documented in the release notes of the next release. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Dec 27, 2017
BurntSushi
BurntSushi requested changes Dec 28, 2017
View reviewed changes
src/libstd/path.rs Outdated
@@ -576,6 +576,13 @@ impl<'a> AsRef<OsStr> for Component<'a> {
}
}

#[unstable(feature = "path_component_asref", issue = "41866")]
Copy link
Member

Choose a reason for hiding this comment

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

Trait impls are insta-stable. Please change this to a stable attribute.

@BurntSushi
Copy link
Member

This seems reasonable to me. @rfcbot fcp merge

@kennytm kennytm added the S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). label Dec 28, 2017
@Diggsey Diggsey force-pushed the path-component-asref branch from 2c2452e to e0855ff Compare December 28, 2017 13:28
@Diggsey
Copy link
Contributor Author

Diggsey commented Dec 28, 2017

@BurntSushi is that correct? Is there any documentation about stability attributes?

@kennytm
Copy link
Member

kennytm commented Dec 28, 2017

(@BurntSushi The @rfcbot fcp merge should be on its own line)

@BurntSushi
Copy link
Member

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Dec 28, 2017 •
edited by aturon
Loading

Team member @BurntSushi has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Dec 28, 2017
@BurntSushi
Copy link
Member

BurntSushi commented Dec 28, 2017 •
edited
Loading

@Diggsey

is that correct? Is there any documentation about stability attributes?

I don't know about any docs. It's just folk knowledge? cc @rust-lang/libs

@sfackler
Copy link
Member

sfackler commented Jan 4, 2018

Yeah, that stability attribute looks right.

@kennytm
Copy link
Member

kennytm commented Jan 10, 2018

Ping @aturon, ticky box for you! #46985 (comment)

@rfcbot
Copy link

rfcbot commented Jan 12, 2018

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Jan 12, 2018
@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Jan 12, 2018

📌 Commit e0855ff has been approved by alexcrichton

kennytm added a commit to kennytm/rust that referenced this pull request Jan 12, 2018
@kennytm
Rollup merge of rust-lang#46985 - Diggsey:path-component-asref, r=ale…
743b976 
…xcrichton

Implement AsRef<Path> for Component

Fixes rust-lang#41866
@kennytm kennytm mentioned this pull request Jan 12, 2018
Merged
bors added a commit that referenced this pull request Jan 12, 2018
@bors
Auto merge of #47392 - kennytm:rollup, r=kennytm
51b0b37 
Rollup of 24 pull requests

- Successful merges: #46985, #47069, #47081, #47185, #47282, #47283, #47288, #47289, #47298, #47305, #47306, #47307, #47310, #47324, #47328, #47331, #47340, #47343, #47344, #47352, #47357, #47365, #47375, #47382
- Failed merges: #47334
@bors bors merged commit e0855ff into rust-lang:master Jan 12, 2018
@jdm jdm mentioned this pull request Jan 15, 2018
Closed
@ExpHP ExpHP mentioned this pull request Jan 15, 2018
Closed
@sfackler sfackler mentioned this pull request Jan 16, 2018
Closed
@Mark-Simulacrum Mark-Simulacrum mentioned this pull request Mar 4, 2018
Closed
@vitiral
Copy link
Contributor

vitiral commented Mar 6, 2018

I'm a little confused why adding an AsRef caused crates to stop compiling. Could there be a brief triage of why that is (and why rust is going forward with breaking changes)?

@BurntSushi
Copy link
Member

@vitiral It's well covered ground, but I actually don't know of any central resource on the matter. AFAIK, if your use of AsRef breaks then it's generally because it is being misused, and in general, it's not feasible to never add new trait impls.

@BurntSushi
Copy link
Member

(From a technicality perspective, we're empowered to make changes like this via the API evolution RFC, but pragmatism rules the day.)

@Diggsey
Copy link
Contributor Author

Diggsey commented Mar 6, 2018

Here is the specific relevant section: https://github.com/rust-lang/rfcs/blob/master/text/1105-api-evolution.md#minor-change-implementing-any-non-fundamental-trait

@Diggsey Diggsey deleted the path-component-asref branch March 6, 2018 16:20
@andreastt andreastt mentioned this pull request Mar 13, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BurntSushi BurntSushi BurntSushi requested changes

Assignees

@BurntSushi BurntSushi

Labels
final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@Diggsey @rust-highfive @BurntSushi @kennytm @rfcbot @sfackler @alexcrichton @bors @vitiral