Skip to content

features: Distinguish between optional dependencies and features #1180

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

Conversation

almusil
Copy link
Contributor

@almusil almusil commented Nov 16, 2020

Hi,

as discussed in #1175, features that are taken from optional dependencies are marked in database and
stored with rename, if provided.

@almusil
Copy link
Contributor Author

almusil commented Nov 16, 2020

@Nemo157 and @jyn514
Frontend now shows correct features after this commit.
async-compression 0.3.6

@jyn514 jyn514 added C-enhancement Category: This is a new feature S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed labels Nov 16, 2020
"Add mark for features that are derived from optional dependencies",
// upgrade query
"
ALTER TYPE feature ADD ATTRIBUTE optional_dependency BOOL;
Copy link
Member

Choose a reason for hiding this comment

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

Where can I learn more about postgres data types? Will optional_dependency be added as NULL? It should be.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, which is an issue when loading the features page for existing crates:

thread '<unnamed>' panicked at 'error retrieving column 0: error deserializing column 0: a Postgres value was `NULL`'

Probably the type needs to be changed to optional_dependency: Option<bool>.

Copy link
Contributor Author

@almusil almusil Nov 16, 2020

Choose a reason for hiding this comment

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

Yes, I have realized that later. IMO it would make sense to make is false as default.

EDIT: Nevermind it seems like we cannot set default in ALTER TYPE. At least I was not able to find it in documentation.

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 we shouldn't say something we don't know for sure. We should show an optional dependency/feature tag if the info is there, but just show nothing if we don't know.

@jyn514 jyn514 changed the title features: Differ optional dependencies and features features: Distinguish between optional dependencies and features Nov 16, 2020
@jyn514 jyn514 added S-waiting-on-author Status: This PR is incomplete or needs to address review comments and removed S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed labels Nov 16, 2020
@almusil almusil force-pushed the differ_optional_dependcies_and_regular_features branch from 51766fb to b65fec0 Compare November 16, 2020 17:40
Store in database if feature comes from optional
dependency. At the same time resolve rename
as optional dependency might use same package
multiple times under different name.
@almusil almusil force-pushed the differ_optional_dependcies_and_regular_features branch from b65fec0 to 2039231 Compare November 16, 2020 18:16
@jyn514 jyn514 merged commit 851ccf7 into rust-lang:master Nov 17, 2020
@jyn514
Copy link
Member

jyn514 commented Nov 17, 2020

Thanks for working on this!

@almusil almusil deleted the differ_optional_dependcies_and_regular_features branch November 17, 2020 09:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: This is a new feature S-waiting-on-author Status: This PR is incomplete or needs to address review comments
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants