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

rustc: rename multiple imports in a list #27451

Merged
merged 1 commit into from
Aug 10, 2015

Conversation

seanmonstar
Copy link
Contributor

An implementation of RFC 1219.

The RFC is not merged yet, but once merged, this could be.

@rust-highfive
Copy link
Collaborator

r? @alexcrichton

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

@@ -2430,11 +2430,11 @@ pub struct ViewListIdent {
impl Clean<ViewListIdent> for ast::PathListItem {
fn clean(&self, cx: &DocContext) -> ViewListIdent {
match self.node {
ast::PathListIdent { id, name } => ViewListIdent {
ast::PathListIdent { id, name, .. } => ViewListIdent {
Copy link
Member

Choose a reason for hiding this comment

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

I think these cases need to be handled to show reexports correctly, this will probably need to update the ViewListIdent structures here, and can you be sure to add a rustdoc test for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OH, I misunderstood what this code did. Re-exports, of course.

@alexcrichton
Copy link
Member

Can you be sure to add some more exhaustive tests for this? It looks like this only includes "this will parse" tests rather than "this will resolve" tests along with tests for things like error messages (e.g. an item doesn't exist in a module).

@seanmonstar
Copy link
Contributor Author

Ah, you're right, that test doesn't test it resolved the correct item.

tests for things like error messages (e.g. an item doesn't exist in a module).

@alexcrichton I assumed those tests already exist as normal importing tests.

@alexcrichton
Copy link
Member

They probably do exist, but new ones should be added for this new feature. This should be as exhaustively tested as possible, basically, because although the implementation may be simple today a later refactoring will benefit from an exhaustive set of tests.

@seanmonstar seanmonstar force-pushed the use-groups-as branch 6 times, most recently from 670aaf6 to e042365 Compare August 5, 2015 19:37
@seanmonstar
Copy link
Contributor Author

@alexcrichton i've fixed up rustdoc and some error tests. some other specific tests you have in mind? I

@Manishearth
Copy link
Member

This needs a visit_ident() in libsyntax/visit.rs

@Manishearth
Copy link
Member

parser-lalr.y probably needs changing to pass tests too

@seanmonstar
Copy link
Contributor Author

@Manishearth a change in walk_item? I don't know the use of that function well; should you visit both the name and rename (if let Some(ident) = rename), or only name is rename is None?

In other words, use std::io::{Error as IoError, Read} would visit Error, IoError, and then Read?

@Manishearth
Copy link
Member

Visit both. visit.rs is for AST visiting, so anything in the AST should get visited

@seanmonstar
Copy link
Contributor Author

@Manishearth like so?

@Manishearth
Copy link
Member

Yep!

@alexcrichton
Copy link
Member

All looks good to me, thanks @seanmonstar!

@bors: r+ cfcd449

bors added a commit that referenced this pull request Aug 10, 2015
An implementation of [RFC 1219](rust-lang/rfcs#1219).

The RFC is not merged yet, but once merged, this could be.
@bors
Copy link
Contributor

bors commented Aug 10, 2015

⌛ Testing commit cfcd449 with merge 8856927...

bors added a commit that referenced this pull request Sep 15, 2015
@bluss bluss added the relnotes Marks issues that should be documented in the release notes of the next release. label Sep 15, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants