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

Sort crate features by name #1320

Closed

Conversation

apiraino
Copy link

@apiraino apiraino commented Mar 17, 2021

This patch changes one detail implemented in #1144. The feature flags
sorting was by "number of subfeatures". Now it is by feature alphabetically
which I find suitable to easily discover the feature I'm looking for in a crate.
It also ensures that the special "default" feature stays on top of the list.

If we don't want to touch this behaviour, I'm also 100% ok to decline.
If there is a better way to implement this I'm happy to improve it.

cc @almusil since author of the original patch
super-thanks to @GuillaumeGomez for mentoring me on this!

r? @GuillaumeGomez or @jyn514

This patch changes one detail implemented in rust-lang#1144. The features flag
sorting was by "number of subfeatures". Now it by feature alphabetically.
It also ensure that the "default" feature stays on top of the list.
} else {
a.name.partial_cmp(&b.name).unwrap()
}
});
Copy link
Author

Choose a reason for hiding this comment

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

at first I've tried to simply change the sorting at the SQL level on line 55 but I think this sorting in this function would neutralize it anyway

@apiraino
Copy link
Author

apiraino commented Mar 17, 2021

How the sorting looks before this patch

old

How the sorting looks after this patch

new

Copy link
Member

@GuillaumeGomez GuillaumeGomez left a comment

Choose a reason for hiding this comment

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

Nice work!

@almusil
Copy link
Contributor

almusil commented Mar 18, 2021

Thank you for including me.

I don't mind this change, I am not sure if the requirement has changed since the initial implementation.
However if we go with this, there is actually no need to compute the whole tree structure etc.
This is maintainers call but I don't see a point in keeping code that is not used.

@apiraino
Copy link
Author

apiraino commented Mar 18, 2021

I don't mind this change, I am not sure if the requirement has changed since the initial implementation.

@almusil good point, I forgot to give some context. Example: I went to the tokio crate feature list (see my screenshots) looking for a specific one and I was confused because I expected them in alphabetical order, so I had to run CTRL+Fand find it.

This may be a subjective "feeling" on how I see the sorting and this PR is just a small question+proposal, this is why I mentioned it's perfectly fine to decline this PR :)

Meta-thought: it's often hard to please every and each user, often there are conflicting thoughts about a UX/UI :) The expectations of a single user (in this case: me) should not put anyone under pressure to accept a PR.

@almusil
Copy link
Contributor

almusil commented Mar 18, 2021

I don't mind this change, I am not sure if the requirement has changed since the initial implementation.

@almusil good point, I forgot to give some context. Example: I went to the tokio crate feature list (see my screenshots) looking for a specific one and I was confused because I expected them in alphabetical order, so I had to run CTRL+Fand find it.

The idea was to have flat tree structure of features that are enabled by default followed by features that enable any other feature.
But having them sorted by name also makes sense in wider context.

This may be a subjective "feeling" on how I see the sorting and this PR is just a small question+proposal, this is why I mentioned it's perfectly fine to decline this PR :)

I understand that, this is worth a discussion.

Meta-thought: it's often hard to please every and each user, often there are conflicting thoughts about a UX/UI :) The expectations of a single user (in this case: me) should not put anyone under pressure to accept a PR.

@jyn514
Copy link
Member

jyn514 commented Mar 20, 2021

Personally I like the tree structure better, I think it shows the features you'd want most often towards the top. If the crate has really complicated features, I think it should be documenting those itself anyway.

@apiraino
Copy link
Author

Personally I like the tree structure better, I think it shows the features you'd want most often towards the top. If the crate has really complicated features, I think it should be documenting those itself anyway.

Based on your reasoning, I think you have a stronger case to keep it as it is, so I'll close my proposal.

(thanks @jyn514 for your thoughts on the matter)

@apiraino apiraino closed this Mar 26, 2021
@apiraino apiraino deleted the sort-crate-features-by-name branch March 26, 2021 07:33
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.

4 participants