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

Turn hash for GapObj into error #891

Merged
merged 1 commit into from
Sep 22, 2023

Conversation

fingolfin
Copy link
Member

... instead of providing a trivial (constant) hash. This may break
some trivial applications, but in return it helps us identify code
that relies on the useless default hash, i.e., it helps us find
performance bottlenecks.

This is a breaking change so also increase the package version.

@codecov
Copy link

codecov bot commented Jun 7, 2023

Codecov Report

Merging #891 (bca8718) into master (6bafdba) will decrease coverage by 0.08%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #891      +/-   ##
==========================================
- Coverage   75.76%   75.69%   -0.08%     
==========================================
  Files          51       51              
  Lines        4160     4160              
==========================================
- Hits         3152     3149       -3     
- Misses       1008     1011       +3     
Files Changed Coverage
src/constructors.jl ø
src/adapter.jl 100.00%

Copy link
Member

@ThomasBreuer ThomasBreuer left a comment

Choose a reason for hiding this comment

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

Do we have to document the problem in general, and give examples of forbidden objects such as Sets of GapObjs?

@fingolfin
Copy link
Member Author

It certainly would be good to discuss the hashing issue somewhere, if only so we can point people at it when they ask about it...

@fingolfin fingolfin closed this Jul 18, 2023
@fingolfin fingolfin reopened this Jul 18, 2023
@ThomasBreuer
Copy link
Member

@fingolfin Shall we merge this pull request, and then I create a new pull request for improving the documentation?

@fingolfin
Copy link
Member Author

This is a breaking change, meaning that once this is merged, the next version should be 0.10.0. But there are already a bunch of other changes merged -- perhaps we can make one more 0.9.x release before merging this -- I've made PR #926 in preparation of this.

... instead of providing a trivial (constant) hash. This may break
some trivial applications, but in return it helps us identify code
that relies on the useless default hash, i.e., it helps us find
performance bottlenecks.

This is a breaking change so also increase the package version.
@fingolfin fingolfin enabled auto-merge (squash) September 22, 2023 15:49
@fingolfin fingolfin merged commit fca8b92 into oscar-system:master Sep 22, 2023
13 checks passed
@fingolfin fingolfin deleted the mh/forbid-hash branch October 6, 2023 09:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants