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

Ignore querystrings #217

Merged
merged 4 commits into from
May 12, 2017
Merged

Conversation

whostolemyhat
Copy link
Contributor

Fix for #207 - strips querystrings from urls when serving files locally

@whostolemyhat
Copy link
Contributor Author

Travis is failing on cargo fmt - I ran the formatter on my code but this produced more errors, so not sure where to go from here.

@epage
Copy link
Member

epage commented May 9, 2017

Thanks for contributing! I probably won't get a chance to review this for a day or two.

Travis is failing on cargo fmt - I ran the formatter on my code but this produced more errors, so not sure where to go from here.

I just ran into the same problem with liquid. The CI code uses a fixed version of rustfmt which is easy to get out of sync with.

https://github.com/cobalt-org/cobalt.rs/blob/master/ci/install.sh#L7

One option is to include updating the version of rustfmt as part of your change. I took the quick and dirty route in liquid and am using cargo to install rustfmt but that slows down the CI. The problem with that is that I want to be respectful of travis/appveyor providing these services for us.

src/main.rs Outdated
// find the path of the file in the local system
// (this gets rid of the '/' in `p`, so the `join()` will not replace the
// path)
let path = PathBuf::from(dest).join(&req_path[1..]);
let path = PathBuf::from(dest).join(&stripped_path[1..]);

Copy link
Member

Choose a reason for hiding this comment

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

Rather than split/collect, what if we

  • rfind + truncate
  • rsplitn(2, "?").last().expect("always at least one element")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool, I'll have a look into this

@epage
Copy link
Member

epage commented May 11, 2017

So what to do about the CI issue.

Personally, I'd recommend stripping off the last of your commits and rebasing against upstream master. At that point it should be resolved if you use the same version of rust fmt (0.8.3 I believe). This will get the CI passing and will keep history clean. I know "rebase + push -f" is generally considered bad form but no else should really be broken because of it and not sure what else would be recommended to do in this case.

If you don't feel up to that, we could probably commit as is (once the issue above gets cleaned up) or I could clone your branch and clean it up if you wish.

@whostolemyhat whostolemyhat force-pushed the ignore-querystrings branch 2 times, most recently from 291b79e to 58ee7f6 Compare May 11, 2017 08:19
@whostolemyhat
Copy link
Contributor Author

I've rebased and deleted the whitespace that fmt was complaining about, and the CI seems happy now.

@whostolemyhat
Copy link
Contributor Author

I've removed the split().collect() and replaced it with rfind - if there's no match then the original req_uri is used.

@epage
Copy link
Member

epage commented May 12, 2017

I've removed the split().collect() and replaced it with rfind - if there's no match then the original req_uri is used.

Looks good. Once the clippy issue is resolved, this is good to go in.

@epage epage merged commit eb9e0b0 into cobalt-org:master May 12, 2017
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.

None yet

2 participants