-
Notifications
You must be signed in to change notification settings - Fork 2
feat: Add allow_unknown_extensions macro param
#8
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
Conversation
b7ec2ca to
51b54e3
Compare
|
Hey @angrynode, thanks for the PR! I don't mind falling back to a default content type when the extension is unknown. The correct value though is |
Good catch, thanks!
I'm not sure i know how to implement this, as i'm not familiar with macros. Maybe i'll find time for this in the next weeks, but if you feel like this feature is useful, don't wait for me :) |
ThinkerDreamer
left a comment
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.
Hi @angrynode! Thank you for your PR! I agree with @paolobarbolini that it might be better to make this a flag inside the existing macros rather than a cargo feature. This was my first proc macro too, and it's not as hard as I thought 😄 So if you want to give it a try, it could be a nice way to learn! You can look at what I did with the other flags ("compress", "cache_bust", "ignore_paths", "strip_html_ext", etc.) to see how to handle adding a new one. I think your infallible_content_type function looks good, just remember to change binary/octet-stream to application/octet-stream like @paolobarbolini mentioned.
5cf4fe4 to
b4d4d2e
Compare
|
OK i've added the macro param. I'm not sure i'm doing everything OK, it's confusing to me what should be expecting a LitBool or a bool. Also i don't think it's specific to this PR, but i find the error confusing: We should probably store the entire filename in the error so it's easier to find the source of the problem, don't you think? If you agree, should i add a commit as part of this PR or open a new PR about this? |
allow_unknown_extensions macro param
b4d4d2e to
f099692
Compare
|
@angrynode I gave it a quick look and everything seems in place. Could you write a couple tests to check that the new parameter works as expected? You can create a new empty (or small) file with a weird extension (or even |
|
As for the Edit: Yeah, I just checked and it shouldn't be too difficult. We can just put the whole |
f099692 to
da296a6
Compare
|
I added a successful test, and a failing test. The failing test had to go in a docstring to use the I also updated the README docs about the new attribute. |
ThinkerDreamer
left a comment
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.
Very nice! I left a few small change requests, mostly typographical errors.
|
Oh, one more thing: could you also add your line about |
fcff27b to
cf520ad
Compare
|
Thanks for the review. I edited accordingly. |
ThinkerDreamer
left a comment
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.
Great! Looks good to me! Thanks again!
Not sure if that's how you want to do things but i find it annoying that my program doesn't compile when there is an unknown extension.
In my daily usage it's not been a great problem but lately i've been working with a person who uses MacOS so there's
.DS_STOREfiles everywhere preventing compilation.in this PR i add an
infalliblefeature flag which uses mimetypebinary/octet-streamwhen it cannot be guessed, instead of crashing compilation.