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

feat: add initial support for brotli #391

Merged
merged 3 commits into from
Sep 5, 2023
Merged

Conversation

JamieMagee
Copy link
Contributor

Closes #390

@isaacs
Copy link
Owner

isaacs commented Jun 16, 2023

(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!

@JamieMagee JamieMagee marked this pull request as ready for review June 20, 2023 04:15
@JamieMagee
Copy link
Contributor Author

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.)

Thanks for the suggestion. I applied this in pack and parse. It results in deeply nested if/else, but it removed entire duplicate branches for brotli.

Consider inferring the brotli-ness based on the file extension in tar.x() and tar.c(), if a file name is specified.

Added a check for .tar.br and .tbr file extensions in parse

@JamieMagee
Copy link
Contributor Author

@isaacs I think this is ready for review now. Thanks!

@JamieMagee
Copy link
Contributor Author

@isaacs if you have a few minutes, could you take another look at this please? Thank you!

@isaacs isaacs closed this in 336fa8f Sep 5, 2023
@isaacs isaacs merged commit 336fa8f into isaacs:main Sep 5, 2023
5 checks passed
@isaacs
Copy link
Owner

isaacs commented Sep 5, 2023

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.

@isaacs
Copy link
Owner

isaacs commented Sep 5, 2023

Welp, CI will pass once #392 is fixed, I guess.

Shipped on 6.2. Thanks!

@JamieMagee JamieMagee deleted the brotli-support branch September 5, 2023 16:41
@JamieMagee
Copy link
Contributor Author

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!

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.

[FEATURE]: expose support for brotli compression
2 participants