Skip to content

factor out codecs and core traits #325

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

koehlma
Copy link

@koehlma koehlma commented Jan 31, 2025

This PR fixes #324.

It introduces two new crates:

  • compression-core: Contains the Decode and Encode traits.
  • compression-codecs: Contains the former codec module.

I ran the changes through cargo semver-checks and it did not complain.

Along the way, I made some decisions that may require further discussion:

  • The root crate async-compression does not expose any of the data structures of the other crates. In particular, I did not move the Level enum and the modules zstd and brotli into compression-codecs. This enables future breaking changes in compression-codecs without breaking async-compression.
  • The crate compression-codecs re-exports the underlying compression libraries. The root crate does not directly depend on any of the libraries anymore. I migrated everything, including the tests, to the re-exported libraries. It may be worthwhile to add additional abstractions to compression-codecs to hide the underlying libraries. However, I think, that this should not be part of this PR, as it is already rather huge.

@koehlma koehlma changed the title factor out codes and core traits factor out codecs and core traits Jan 31, 2025
Copy link
Collaborator

@NobodyXu NobodyXu left a comment

Choose a reason for hiding this comment

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

Thank you!

Some feedback on the code

@robjtede robjtede added the A-semver-major breaking change requiring a major version bump label May 10, 2025
@bryantbiggs
Copy link

@koehlma any plans to continue on this PR?

@koehlma
Copy link
Author

koehlma commented May 23, 2025

Yeah, I still think it makes sense to separate those out. I would need to have a look at the changes that happened in the meantime, as they seem to break part of this PR. There was only one failing test (prior to the conflicting changes), however, I did not understand why it was failing. It would be great if there was someone who would look into that, if that were to happen again, so that we can see this through together.

I do not quite understand the A-semver-major label: This PR is designed to not be a breaking change. At least this was the intention and I don't see where it would break semver compatibility.

@NobodyXu
Copy link
Collaborator

I do not quite understand the A-semver-major label

I think it might be due to new crate added.

1 similar comment
@NobodyXu
Copy link
Collaborator

I do not quite understand the A-semver-major label

I think it might be due to new crate added.

@NobodyXu
Copy link
Collaborator

Can you resolve the conflicts please, so that it would trigger another CI run?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-semver-major breaking change requiring a major version bump
Projects
None yet
Development

Successfully merging this pull request may close these issues.

factor out codec into a separate crate
4 participants