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.

Sign up for GitHub

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

Jump to bottom

Implement --route-prefix to set specific route prefix #682

Closed
wants to merge 5 commits into from
Closed

Implement --route-prefix to set specific route prefix #682

wants to merge 5 commits into from

Conversation

Jikstra
Copy link
Contributor

@Jikstra Jikstra commented Dec 29, 2021 •
edited
Loading

Fixes #600

@Jikstra Jikstra mentioned this pull request Dec 29, 2021
Closed
Jikstra
Jikstra commented Dec 29, 2021
View reviewed changes
tests/serve_request.rs
#[rstest]
fn serves_requests_with_path_prefix(
#[with(["--route-prefix", "foobar"])] server: TestServer,
) -> Result<(), Error> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

On the old pr @aliemjay mentioned having more test cases. I don't really understand that case macro, the first two booleans make me wonder. Can someone help me with this?

aliemjay
aliemjay approved these changes Dec 29, 2021
View reviewed changes
Copy link
Contributor

@aliemjay aliemjay left a comment

Choose a reason for hiding this comment

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

Other than a few nitpicks :) it looks good.

src/main.rs Outdated Show resolved Hide resolved
src/config.rs Outdated Show resolved Hide resolved
Jikstra and others added 3 commits January 3, 2022 20:14
@Jikstra @aliemjay
Update src/config.rs
d64b254 
Co-authored-by: Ali MJ Al-Nasrawy <alimjalnasrawy@gmail.com>
@Jikstra
Apply requested changes
020516c
@Jikstra
Merge branch 'feat_route_prefix' of github.com:Jikstra/miniserve into…
4c143d2 
… feat_route_prefix
@Jikstra
Copy link
Contributor Author

Jikstra commented Jan 3, 2022

@aliemjay thank you, applied your requested changes :)

@Jikstra
Copy link
Contributor Author

Jikstra commented Jan 4, 2022 •
edited
Loading

@svenstaro can you have a look? Not so much in the mood to rebase this another time. Also, a little bit off topic, but are you interested in having more maintainers for this project?

aliemjay
aliemjay suggested changes Jan 4, 2022
View reviewed changes
src/config.rs Outdated Show resolved Hide resolved
@Jikstra @aliemjay
Apply alimjays suggestion
48f192e 
Co-authored-by: Ali MJ Al-Nasrawy <alimjalnasrawy@gmail.com>
@Shados
Copy link

Shados commented Jan 5, 2022

@Jikstra The dynamically-generated css_route and favicon_route need to be prefixed, as well.

@Shados
Copy link

Shados commented Jan 5, 2022

Also, the route prefix should be appended to the root dir breadcrumb, for the generated directory listing page.

@svenstaro
Copy link
Owner

@svenstaro can you have a look? Not so much in the mood to rebase this another time. Also, a little bit off topic, but are you interested in having more maintainers for this project?

Looking now. Yes, I'd rather appreciate more maintainers on this project if you're up. Do you have any chat communication channels so that we can coordinate?

svenstaro
svenstaro approved these changes Feb 4, 2022
View reviewed changes
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.

Thanks, this is great. Sorry for delaying for so long.

svenstaro
svenstaro requested changes Feb 4, 2022
View reviewed changes
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.

I found the same issues that @Shados found, apart from that it seems good to merge for me.

@Jikstra
Copy link
Contributor Author

Jikstra commented Feb 5, 2022 •
edited
Loading

@svenstaro can you have a look? Not so much in the mood to rebase this another time. Also, a little bit off topic, but are you interested in having more maintainers for this project?

Looking now. Yes, I'd rather appreciate more maintainers on this project if you're up. Do you have any chat communication channels so that we can coordinate?

I will be busy until march, but then I'm up. You can send me a message on ****

aliemjay added a commit to aliemjay/miniserve that referenced this pull request Feb 6, 2022
@aliemjay
Merge 'jikstra/feat_route_prefix' svenstaro#682
8e0930c
@aliemjay aliemjay mentioned this pull request Feb 6, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aliemjay aliemjay aliemjay requested changes

@svenstaro svenstaro svenstaro requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

give a solid_route
4 participants
@Jikstra @Shados @svenstaro @aliemjay