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

bugfix : skip doc(hidden) default members #15050

Merged
merged 4 commits into from Aug 1, 2023
Merged

Conversation

alibektas
Copy link
Member

@alibektas alibektas commented Jun 13, 2023

fixes #14957 . I have two questions :

  1. I am definitely looking for a more idiomatic way for the things I added in crates/ide-assists/src/utils.rs. See FIXME in that file.
  2. Would it be actually better to change DefaultMethods to something like
enum DefaultMethods {
     Only( IgnoreHidden ( bool ) ) ,
     None
}

instead of adding a boolean to every function that calls crates/ide-assists/src/utils.rs::filter_assoc_items

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 13, 2023
Copy link
Member

@Veykril Veykril left a comment

Choose a reason for hiding this comment

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

I don't think we'll ever want to not ignore doc(hidden) assoc items here so we can drop the ignore_hidden: bool, param.

crates/ide-assists/src/utils.rs Outdated Show resolved Hide resolved
crates/ide-assists/src/utils.rs Outdated Show resolved Hide resolved
@lowr
Copy link
Contributor

lowr commented Jun 14, 2023

I don't think we'll ever want to not ignore doc(hidden) assoc items here so we can drop the ignore_hidden: bool, param.

I initially thought the same thing, but on a second thought, you may want them when they're in local crates as opposed to external crates.

@alibektas
Copy link
Member Author

alibektas commented Jun 14, 2023

I don't think we'll ever want to not ignore doc(hidden) assoc items here so we can drop the ignore_hidden: bool, param.

I initially thought the same thing, but on a second thought, you may want them when they're in local crates as opposed to external crates.

From the thumbs up I gather that we do want to have some sort of guard and this should in addition check that the original trait is a local one if we want to skip ignoring items with doc(hidden) attr. So my question is how you know if the original crate is a local?
I came up with this but there is something unlikable about it :) :

hir::AssocItem::containing_trait(assoc_item, db)?
                .module(db)
                .krate()
                .origin(db)
                .is_local();

@Veykril
Copy link
Member

Veykril commented Jun 20, 2023

That seems fine for checking if the crate is local

@alibektas
Copy link
Member Author

alibektas commented Jun 23, 2023

This PR is almost finished, the last problem I have is with declaring a crate as workspace crate using test-utils::fixture. See FIXME in add_missing_impl_mems. Once I understand that I want to add two more tests, one for workspace crates and one for lib crates, after which this PR will be ready for a merge.

@alibektas alibektas marked this pull request as ready for review June 23, 2023 16:58
@lowr
Copy link
Contributor

lowr commented Jun 24, 2023

I think what you want is new_source_root meta. See this test and its following tests for example.

@Veykril
Copy link
Member

Veykril commented Jul 3, 2023

@alibektas did you take a look at the new_source_root thing?

@Veykril Veykril added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 3, 2023
@alibektas
Copy link
Member Author

Yes I did. But I couldn't really do what I wanted to, which I though could have been fixed with this PR . It was however not merged the last time I checked it. So now that it's merged I will finish this one either today or tomorrow the latest.

@alibektas
Copy link
Member Author

@rustbot label -S-waiting-on-author +S-waiting-on-review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 6, 2023
@alibektas
Copy link
Member Author

I believe this is ready to be merged.

@Veykril
Copy link
Member

Veykril commented Aug 1, 2023

@bors r+

@bors
Copy link
Collaborator

bors commented Aug 1, 2023

📌 Commit 915ddb0 has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Aug 1, 2023

⌛ Testing commit 915ddb0 with merge efc5a81...

@bors
Copy link
Collaborator

bors commented Aug 1, 2023

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing efc5a81 to master...

@bors bors merged commit efc5a81 into rust-lang:master Aug 1, 2023
10 checks passed
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Respect #[doc(hidden)] when generating trait impls
6 participants