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

Amend RFC 1270 to describe actual implementation #1423

Merged
merged 1 commit into from
Feb 4, 2016

Conversation

durka
Copy link
Contributor

@durka durka commented Dec 22, 2015

Change reason to note and remove use.

This isn't a new RFC, just some edits to reflect reality. rust-lang/rust#30206 (comment)

Change `reason` to `note` and remove `use`.
@llogiq
Copy link
Contributor

llogiq commented Dec 23, 2015

👍

@llogiq
Copy link
Contributor

llogiq commented Dec 23, 2015

note that use or some other extension may still be in future Rust.

@durka
Copy link
Contributor Author

durka commented Dec 23, 2015

Yes I only removed it from the example code, not from the Alternatives section.

@liigo
Copy link
Contributor

liigo commented Dec 25, 2015

to describe actual implementation

Implementation should obey the RFC, not 'vice versa'. So maybe you would give an actual reason to make this change.

@llogiq
Copy link
Contributor

llogiq commented Dec 25, 2015

As I said in my other (now closed because of git history issues) PR, this is a minor change, which (as written on the RFC itself) reflects actual usage of the internal feature better, which is a good indicator of the needs of library authors.

@liigo
Copy link
Contributor

liigo commented Dec 28, 2015

@llogiq

reflects actual usage of the internal feature better

could you elaborate?

As I said before (in other PR), I'm not object to this change, just ask for writing down why we change this, in commit messages, so people can trace it later.

FWIW, what I means 'the RFC' is the RFC that was merged into rust-lang/rfcs by libs team, not the rfc PR in your own repo.

@llogiq
Copy link
Contributor

llogiq commented Dec 28, 2015

As for the rationale, look at current usage of the rustc-internal deprecation feature. Though the field was named reason, it often contained e.g. directions on what to use else instead of the actual reason for the deprecation. Since current use is so diverse, choosing a less specific name conveys that the field can be broadly used and leaves us free to choose to add a reason field later should we deem it useful.

@nrc nrc added the T-libs-api Relevant to the library API team, which will review and decide on the RFC. label Jan 4, 2016
@alexcrichton
Copy link
Member

🔔 This RFC is now entering its week-long final comment period 🔔

@alexcrichton alexcrichton added the final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. label Jan 29, 2016
@alexcrichton
Copy link
Member

The libs team discussed this today and the conclusion was to merge, thanks @durka!

alexcrichton added a commit that referenced this pull request Feb 4, 2016
Amend RFC 1270 to describe actual implementation
@alexcrichton alexcrichton merged commit 502ecca into rust-lang:master Feb 4, 2016
@durka
Copy link
Contributor Author

durka commented Feb 4, 2016

My first RFC!

@llogiq
Copy link
Contributor

llogiq commented Feb 4, 2016

Yay! 👍

@Centril Centril added A-attributes Proposals relating to attributes A-diagnostics Proposals relating to diagnostics (error messages). A-lint Proposals relating to lints / warnings / clippy. and removed A-diagnostics Proposals relating to diagnostics (error messages). labels Nov 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-attributes Proposals relating to attributes A-lint Proposals relating to lints / warnings / clippy. final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. T-libs-api Relevant to the library API team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants