feat: add initial support for brotli #391
Conversation
|
(On mobile, may do a fuller review later) Rather than repeat the zip setup code, consider DRYing by choosing the constructor based on config, if those lines are otherwise identical. (If the setup isn't identical, of course that's not an option.) Consider inferring the brotli-ness based on the file extension in tar.x() and tar.c(), if a file name is specified. Otherwise this looks like a great start! |
Thanks for the suggestion. I applied this in
Added a check for |
|
@isaacs I think this is ready for review now. Thanks! |
|
@isaacs if you have a few minutes, could you take another look at this please? Thank you! |
PR-URL: isaacs#391 Credit: @JamieMagee Close: isaacs#391 Reviewed-by: @isaacs
isaacs
force-pushed
the
brotli-support
branch
from
9e5b4b1
to
336fa8f
Compare
|
Sorry for the delay, this has been bubbling up my stack, but it's competing with a ton of work getting tap v18 over the finish line. I played with this a bit more, and got to thinking, a filename can be kind of a flimsy indicator that , even though we don't have magic bytes to determine whether something definitely is brotli, the file is going to be one of only three options: gzip, brotli, or tar. So, if the first 512 bytes are a valid tar header, then ignore the filename, because the chances of a valid tar header also being a valid brotli stream are astronomical, so it's far more likely that the filename is just misleading. Will get this shipped with my change as soon as ci can chew through it. |
|
Welp, CI will pass once #392 is fixed, I guess. Shipped on 6.2. Thanks! |
|
Thanks for reviewing and merging, and thank you for the additional changes. Sorry, I thought it had dropped off your radar. I hope the tap v18 work goes as smoothly! |
Closes #390