-
Notifications
You must be signed in to change notification settings - Fork 165
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
Implement configuration for automatic local bundle loading. #211
Conversation
Thanks for submitting this as well! I like the general idea of this PR, but having a boolean |
7af6640
to
811207d
Compare
Ok, so i changed it to try to detect an |
Codecov Report
@@ Coverage Diff @@
## master #211 +/- ##
==========================================
- Coverage 37.9% 37.89% -0.01%
==========================================
Files 131 131
Lines 60307 60339 +32
==========================================
+ Hits 22857 22864 +7
- Misses 37450 37475 +25
Continue to review full report at Codecov.
|
@pkgw Please review this when you have time. What can i do to improve code coverage in this case? |
src/config.rs
Outdated
let url = &self.default_bundles[0].url; | ||
const FILE_SCHEME_PREFIX: &'static str = "file://"; | ||
if url.starts_with(FILE_SCHEME_PREFIX) { | ||
return Ok(self.make_local_file_provider(&url[FILE_SCHEME_PREFIX.len()..], status)?); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am a little concerned about URL encoding here. A proper URL might contain a sequence like %20
to encode a space, but this approach won't convert it as appropriate. I think the solution here is to parse the URL for real — Tectonic already depends on the url
crate, so that should be used. Then you can check if the scheme
is file
, and use the handy to_file_path function.
OK, this looks very good. There is one change I think should be made to make the URL parsing more correct but based on my read of the relevant API docs, I think it should be easy to implement. It might be hard to boost the code coverage here. A small unit test could maybe cover how different URLs are parsed, but since these functions want to do the actual I/O, I suspect that one would need to build a lot of infrastructure to have a test cover all of the code. It is OK if you can't figure out a way to get code coverage for your changes. If you can devise a small new test case that increases coverage in some other part of the codebase, even if it is totally unrelated to this change, that would be great, but I'll be happy to merge this even if the coverage doesn't go up. |
Thanks for addressing this! I had to merge in changes from another PR to master, but will merge this once the CI passes. |
OK, as expected, code coverage remains a challenge, but let's get this merged. |
pdf_new_number
This commit adds an optional
local
parameter inconfig.toml
, when specified astrue
, tectonic will try to load bundle from a zip file path specified in the usualurl
parameter.