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

Makes ion-hash a feature of ion-rs #478

Merged
merged 1 commit into from
Mar 13, 2023
Merged

Conversation

popematt
Copy link
Contributor

@popematt popematt commented Mar 10, 2023

Issue #, if available:

Resolves #454

Description of changes:

  • Moves ion-hash from its own crate into ion-rs
  • Moves the ion-hash-tests submodule to the root of the repo
  • Updates the GHA workflow to run cargo tests with no features and with all features.
  • Creates a new version of ion-hash crate to essentially tombstone the package.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@codecov
Copy link

codecov bot commented Mar 10, 2023

Codecov Report

Patch coverage: 80.00% and no project coverage change.

Comparison is base (f49659e) 90.14% compared to head (dda6d3f) 90.14%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #478   +/-   ##
=======================================
  Coverage   90.14%   90.14%           
=======================================
  Files          76       77    +1     
  Lines       13568    13568           
=======================================
  Hits        12231    12231           
  Misses       1337     1337           
Impacted Files Coverage Δ
ion-hash/src/lib.rs 100.00% <ø> (ø)
src/ion_hash/element_hasher.rs 98.00% <ø> (ø)
src/ion_hash/representation.rs 99.00% <ø> (ø)
src/ion_hash/type_qualifier.rs 100.00% <ø> (ø)
src/lib.rs 87.22% <ø> (ø)
tests/ion_hash_tests.rs 80.98% <50.00%> (ø)
src/ion_hash/mod.rs 100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@popematt popematt requested a review from zslayton March 10, 2023 07:01
Copy link
Contributor

@zslayton zslayton left a comment

Choose a reason for hiding this comment

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

yessss 🚀

One question about optional dependencies below. If it's straightforward, can you address the clippy warnings before merging? (With the exception of the redundant closure warning, which is caused by issue #472.)

@@ -27,6 +27,10 @@ members = [
"ion-hash"
]

[features]
default = []
ion-hash = ["digest"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this mean that the optional digest dependency should be included when the ion-hash feature is enabled? If so, should sha2 also be in this list?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This means that ion-hash will automatically bring in digest. I don't think it should bring in sha2 because sha2 is not required. Someone might want to use md5 or another hash function instead.

@popematt popematt merged commit 9019a2e into amazon-ion:main Mar 13, 2023
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.

Consider making ion-hash an optional feature in ion-rs
3 participants