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

Allow banning or requiring specific features #253

Closed
wants to merge 17 commits into from

Conversation

Stupremee
Copy link
Contributor

Resolves #226

@Stupremee Stupremee marked this pull request as draft August 31, 2020 11:08
@Stupremee
Copy link
Contributor Author

Stupremee commented Sep 3, 2020

This will take some more time (~2-4 weeks probably) because school started.

@Jake-Shadle
Copy link
Member

No worries, take your time!

@Stupremee Stupremee marked this pull request as ready for review September 26, 2020 07:58
@Jake-Shadle
Copy link
Member

@Stupremee sorry it took a while to look at this, but it looks good, the only thing missing is a few tests for this!

@Stupremee
Copy link
Contributor Author

Yes. I will I add them in the next few days

@Jake-Shadle Jake-Shadle mentioned this pull request Oct 20, 2020
@Jake-Shadle
Copy link
Member

So I was hoping to get this merged in and added to the next release, but it's looking like a bigger change than I initially thought, so I'm going to just do a release now, promise this will get in soon!

@memoryruins
Copy link

@Jake-Shadle is there still interest in adding this feature?

@Jake-Shadle
Copy link
Member

There is, there were just some bugs/missing parts in this PR and I just haven't had time to come back to it and clean it up unfortunately, but it is definitely something we want.

@repi repi requested a review from Jake-Shadle January 30, 2022 13:11
@Stupremee
Copy link
Contributor Author

Stupremee commented Jun 21, 2022

Hey @Jake-Shadle!

This Pull Request is almost two years old, but I'm sure there's still interest for this feature.
If you'd like, I can update and rework this PR to your interests, fix the bugs and clean the code up a bit.

@Jake-Shadle
Copy link
Member

So the reason I never merged this was not because the PR itself didn't work, but rather that currently the printing of the reverse crate dependency graphs when showing diagnostics has no notion of features, and it felt like this feature would be incomplete without that addition. But that being said, I think you could update this PR to compile again (or maybe just create a new one from HEAD) to add this feature, and we could get it in and then add the better crate graph reporting separately. Also this might have some interaction with EmbarkStudios/krates#41 but not sure.

@Jake-Shadle
Copy link
Member

Supplanted by #434

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow banning or requiring specific features.
3 participants