Skip to content

Implement AsRef<Path> for Component#46985

Merged
bors merged 1 commit intorust-lang:masterfrom
Diggsey:path-component-asref
Jan 12, 2018
Merged

Implement AsRef<Path> for Component#46985
bors merged 1 commit intorust-lang:masterfrom
Diggsey:path-component-asref

Conversation

@Diggsey
Copy link
Contributor

@Diggsey Diggsey commented Dec 24, 2017

Fixes #41866

@rust-highfive
Copy link
Contributor

rust-highfive commented Dec 24, 2017

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

@BurntSushi BurntSushi Dec 28, 2017

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

BurntSushi commented Dec 28, 2017

This seems reasonable to me. @rfcbot fcp merge

@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

BurntSushi commented Dec 28, 2017

@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

alexcrichton commented Jan 12, 2018

@bors: r+

@bors
Copy link
Collaborator

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

BurntSushi commented Mar 6, 2018

@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

BurntSushi commented Mar 6, 2018

(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. 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