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

Remove redundant 'ElementConstraint' #170

Merged
merged 2 commits into from
May 21, 2018

Conversation

effectfully
Copy link
Contributor

ElementConstraint gives some flexibility, but it doesn't seem it's needed. Having the Eq (Element t) constraint for the elem function looks like a sensible prerequisite -- I doubt people would define their own equality type classes and use them instead of Eq. And if elem requires something else than Eq (like Hashable), then this also should be required for constructing such data structure, so you can just constrain the entire instance like this:

instance (Eq v, Hashable v) => Container (HashSet v) where

@chshersh
Copy link
Contributor

@effectfully Thanks a lot for your contribution! This looks like a descent improvement which increases conveniency. Could you also please update CHANGELOG and confirm that benchmarks for elem function work as expected?

Copy link
Contributor

@chshersh chshersh left a comment

Choose a reason for hiding this comment

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

Changelog + benchmarks

@effectfully
Copy link
Contributor Author

Added a note to the changelog. Unfortunately, I can't check the benchmarks, because I'm constantly getting severely inflated results. Might be a problem with my machine.

@@ -1,9 +1,11 @@
1.2.0
=====

* [#170](https:/serokell/universum/pull/170):
Remove `ElementConstraint` from the `Container` class.
Copy link
Member

Choose a reason for hiding this comment

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

1.2.0 is already on Hackage, so this changelog entry is misplaced. It should probably go to 1.3.0. @chshersh?

Copy link
Contributor

Choose a reason for hiding this comment

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

@gromakovsky Yes, this should go to 1.3.0. I don't want to bother @effectfully with maintaining changelog properly since it involves knowing specific version policy for the project, etc. I can move this line to 1.3.0 w/o problems but I'm just to lazy to write everything by my own...

@chshersh chshersh merged commit 5e6b507 into serokell:master May 21, 2018
@chshersh
Copy link
Contributor

I checked benchmarks by myself. Everything is still working as expected 👌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants