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.

Sign up for GitHub

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

Jump to bottom

refactor: make hashbrown dependency optional, hashbrown feature #155

Merged
merged 3 commits into from
Jun 22, 2023
Merged

refactor: make hashbrown dependency optional, hashbrown feature #155

merged 3 commits into from
Jun 22, 2023

Conversation

dj8yfo
Copy link
Collaborator

@dj8yfo dj8yfo commented Jun 20, 2023 •
edited by frol

This PR is a small fraction of #51

Resolves #45

@dj8yfo dj8yfo requested a review from frol as a code owner June 20, 2023 07:12
@dj8yfo dj8yfo force-pushed the hashbrown_under_feature branch 2 times, most recently from 8759e7b to 24f8918 Compare June 20, 2023 07:33
frol
frol reviewed Jun 20, 2023
View reviewed changes
.github/workflows/rust.yml Outdated Show resolved Hide resolved
@dj8yfo dj8yfo mentioned this pull request Jun 20, 2023
Closed
24 tasks
@dj8yfo dj8yfo changed the title chore: make hashbrown dependency optional, external-hashcollections feature chore: make hashbrown dependency optional, hashbrown feature Jun 20, 2023
@dj8yfo dj8yfo force-pushed the hashbrown_under_feature branch 3 times, most recently from 4672d0e to b2325e4 Compare June 20, 2023 18:13
@dj8yfo
Copy link
Collaborator Author

dj8yfo commented Jun 20, 2023 •
edited

The following has probably to be done in addition to changes already included:

  • add annotation to all of integration tests in borsh #![cfg_attr(not(feature = "std"), no_std)]
    • smoke.rs
    • test_arrays.rs
    • test_binary_heaps.rs
    • test_bson_object_ids.rs
    • test_custom_reader.rs
    • test_de_errors.rs
    • test_generic_struct.rs
    • test_hash_map.rs
    • test_macro_namespace_collisions.rs
    • test_nonzero_integers.rs
    • test_primitives.rs
    • test_rc.rs
    • test_schema_enums.rs
    • test_schema_nested.rs
    • test_schema_primitives.rs
    • test_schema_structs.rs
    • test_schema_tuple.rs
    • test_simple_structs.rs
    • test_strings.rs
    • test_tuple.rs
    • test_vecs.rs
    • test_zero_size.rs

@dj8yfo dj8yfo force-pushed the hashbrown_under_feature branch 4 times, most recently from b91208a to 0a2377b Compare June 20, 2023 19:09
frol
frol requested changes Jun 21, 2023
View reviewed changes
borsh/src/schema.rs Outdated Show resolved Hide resolved
borsh/src/ser/mod.rs Outdated Show resolved Hide resolved
borsh/tests/test_simple_structs.rs Outdated Show resolved Hide resolved
borsh/src/de/mod.rs Outdated Show resolved Hide resolved
.github/workflows/rust.yml Outdated Show resolved Hide resolved
@dj8yfo dj8yfo force-pushed the hashbrown_under_feature branch 10 times, most recently from 7e33c4d to da9a3ba Compare June 21, 2023 12:08
dj8yf0μl added 2 commits June 21, 2023 15:23
chore(ci): move Run tests actions to test file (to allow running loca…
f183b40 
…lly)
chore: making --no-default-features build/test with hashbrown extra…
f4d5712 
…cted under a feature

- the whole module with `BorshSchema` trait had to be put under conditional compilation due to it using `HashMap`/`HashSet` in its implementation
@dj8yfo dj8yfo force-pushed the hashbrown_under_feature branch 19 times, most recently from 6edac13 to f4d5712 Compare June 22, 2023 09:59
frol
frol approved these changes Jun 22, 2023
View reviewed changes
Copy link
Collaborator

@frol frol left a comment

Choose a reason for hiding this comment

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

@dj8yfo Well done! Thanks for the thorough work diving deep down into this rabbit hole

@frol
Copy link
Collaborator

frol commented Jun 22, 2023

Just for future reference, this PR is a breaking change:

  • for nostd users as they would now need to explicitly enable hashbrown feature if they use HashMap / HashSet collections.
  • for those who used to explicitly specify const-generics feature, which was soft-deprecated some time ago, and did not do anything in reality as const-generics used in borsh are stabilized in MSRV borsh require anyway.

@frol frol mentioned this pull request Jun 22, 2023
Closed
@frol
Copy link
Collaborator

frol commented Jun 22, 2023

@dj8yfo It would be great if you can take #158

@frol frol merged commit 27f38ed into near:master Jun 22, 2023
5 checks passed
@frol frol changed the title chore: make hashbrown dependency optional, hashbrown feature refactor: make hashbrown dependency optional, hashbrown feature Jun 22, 2023
@frol frol mentioned this pull request Jun 22, 2023
Closed
@dj8yfo dj8yfo deleted the hashbrown_under_feature branch June 22, 2023 16:58
@dj8yfo dj8yfo mentioned this pull request Aug 2, 2023
Merged
@frol frol mentioned this pull request Aug 9, 2023
Closed
@willhickey willhickey mentioned this pull request Apr 3, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@frol frol frol approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove mandatory dependency on hashbrown
2 participants
@dj8yfo @frol