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

Set default sorting order and method with arguments #1308

Merged
merged 1 commit into from
Jan 13, 2024
Merged

Set default sorting order and method with arguments #1308

merged 1 commit into from
Jan 13, 2024

Conversation

ElliottLandsborough
Copy link
Contributor

@ElliottLandsborough ElliottLandsborough commented Jan 9, 2024

My intention is to add a way to sort by descending date (or anything else) by default.

I think the best way to do this is with a command line argument but I am open to suggestions.

This is useful for anyone who wants the most recently created/edited files at the top of the list when the page first loads.

Without this change, a user would have to do an entire whole extra single click to sort the files by date 😢

@svenstaro
Copy link
Owner

Cool! Could you add a description for what it is supposed to do and why? For example, why aren't the current sorters not sufficient? I suppose the idea is to provide a sorting default?

@ElliottLandsborough
Copy link
Contributor Author

Cool! Could you add a description for what it is supposed to do and why? For example, why aren't the current sorters not sufficient? I suppose the idea is to provide a sorting default?

I have updated the description - you are right, I want to be able to set the default sort type and order.

The chevrons may have been pointing in the wrong direction too so I have included the change. This is not a big deal to me and it is probably open to interpretation.

@svenstaro
Copy link
Owner

Code looks perfect. Can I somehow convince you to write a test for this?

@ElliottLandsborough ElliottLandsborough changed the title Set sorting order and method with arguments Set default sorting order and method with arguments Jan 10, 2024
@ElliottLandsborough
Copy link
Contributor Author

Code looks perfect. Can I somehow convince you to write a test for this?

I've added a test, fixed some bugs, updated some comments.

@ElliottLandsborough ElliottLandsborough marked this pull request as ready for review January 10, 2024 21:10
Copy link
Owner

@svenstaro svenstaro left a comment

Choose a reason for hiding this comment

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

Looks great, just one comment about the tests.

tests/navigation.rs Show resolved Hide resolved
tests/navigation.rs Outdated Show resolved Hide resolved
@svenstaro
Copy link
Owner

I'm happy to merge this and release a new version of miniserve once the test is a bit more useful.

@ElliottLandsborough
Copy link
Contributor Author

I'm happy to merge this and release a new version of miniserve once the test is a bit more useful.

Tests have been updated 😃

@@ -12,3 +12,19 @@ pub fn get_link_from_text(document: &Document, text: &str) -> Option<String> {
.next()?;
Some(a_elem.attr("href")?.to_string())
}

/// Return the href attributes of all links that start with the specified prefix `text`.
pub fn get_link_hrefs_from_text_with_prefix(document: &Document, text: &str) -> Vec<String> {
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
pub fn get_link_hrefs_from_text_with_prefix(document: &Document, text: &str) -> Vec<String> {
pub fn get_link_hrefs_from_text_with_prefix(document: &Document, prefix: &str) -> Vec<String> {

@@ -12,3 +12,19 @@ pub fn get_link_from_text(document: &Document, text: &str) -> Option<String> {
.next()?;
Some(a_elem.attr("href")?.to_string())
}

/// Return the href attributes of all links that start with the specified prefix `text`.
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
/// Return the href attributes of all links that start with the specified prefix `text`.
/// Return the href attributes of all links that start with the specified `prefix`.

#[case(server(&["--default-sorting-method", "date", "--default-sorting-order", "asc"]))]
/// We can specify the default sorting order
fn can_specify_default_sorting_order(#[case] server: TestServer) -> Result<(), Error> {
let slash = String::from("/");
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not sure how I feel about this since we only really need it at a single place. Maybe just create this where it needs to be and remove this?

@svenstaro
Copy link
Owner

I'm making the last few bits of changes myself. Thanks for the contribution!

@svenstaro svenstaro merged commit c78db05 into svenstaro:master Jan 13, 2024
14 of 17 checks passed
svenstaro added a commit that referenced this pull request Jan 13, 2024
@ElliottLandsborough
Copy link
Contributor Author

Thanks - that was the fastest i've ever had a PR merged to anyone else's project 👍

@svenstaro
Copy link
Owner

Happy to hear! Check my changes to your code as well. I think I improved it somewhere and it's now pretty neat.

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.

2 participants