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

Suggestion: remove Semiring Validation from core #27

Open
hdgarrood opened this issue Oct 1, 2020 · 7 comments
Open

Suggestion: remove Semiring Validation from core #27

hdgarrood opened this issue Oct 1, 2020 · 7 comments
Labels
purs-0.15 A reminder to address this issue or merge this PR before we release PureScript v0.15.0 type: breaking change A change that requires a major version bump.

Comments

@hdgarrood
Copy link
Contributor

For some code to be part of the core libraries, I think there should be quite a high bar to meet. The core team should be able to understand and maintain it, it should be robust and correct, and it should be something we expect people to want to use regularly in their programs. I'm not sure Data.Validation.Semiring meets that bar. I certainly don't understand it - I don't understand why you would want to be able to combine errors in two different ways like that. It has also proven resistant to efforts to document it. Additionally, the type which you're meant to use for accumulating errors, Data.Semiring.Free, isn't even a real Semiring: purescript/purescript-semirings#11.

Given all of these things, I think Data.Validation.Semiring (and probably Data.Semiring.Free too) would be best moved to a separate non-core library.

@JordanMartinez
Copy link
Contributor

Makes sense to me, but where would it go? purescript-contrib?

Also, do any other purescript libs depend on this repo?

@hdgarrood
Copy link
Contributor Author

I'm sure other libraries depend on this library, but I imagine that the vast majority of those only depend on Data.Validation.Semigroup, not the Semiring part. I'd suggest not putting it in contrib unless we can identify a maintainer who does feel able to maintain it; I feel like moving it to someone's personal account might be best. @paf31 @cryogenian do either of you have thoughts on this?

@kl0tl
Copy link
Member

kl0tl commented Nov 26, 2020

The semiring structure on errors is required to satisfy the annihilation law (empty <*> rhs = empty) of Alternative:

Left zero <*> Left failure = Left (zero * failure) = Left zero

A semigroup is insufficient:

Left zero <*> Left failure = Left (zero <> failure) = Left failure

Data.Validation.Semigroup.V has no Alt nor Plus instances, but they could be lawful. Neither type can implement Alternative though because they don’t satisfy the distributivity law ((f <*> a) <|> (g <*> a) = (f <|> g) <*> a), but Data.Validation.Semiring.V could if we dropped the law (which seems to be the direction we’re taking in purescript/purescript-control#63).

So I think that whether to keep Data.Validation.Semiring.V or not might depends on what we do about Alternative and its laws?

Also, is there any reason for not having Alt and Plus (unless we merge Plus into Alternative) instances for Data.Validation.Semigroup.V?

@hdgarrood
Copy link
Contributor Author

I don't really understand what you're getting at, is it that you think we should consider keeping the Semiring validation because it might be able to support an Alternative instance (whereas the Semigroup version can't)? For me that's very much a secondary concern after the concerns I raised in my initial comment.

@cryogenian
Copy link
Contributor

Ps-routing from contrib uses semiring. Maybe move it to contrib or even make it part of routing lib, if noone needs it?

@kl0tl
Copy link
Member

kl0tl commented Dec 14, 2020

I don't really understand what you're getting at, is it that you think we should consider keeping the Semiring validation because it might be able to support an Alternative instance (whereas the Semigroup version can't)?

I don’t have much of an opinion about this, I just wanted to document a subtle difference between the two Validation types ^^

@hdgarrood
Copy link
Contributor Author

I wasn't aware that purescript-routing uses semiring validation, thanks for making me aware of that. At least we have a motivating example now. I'd like to investigate whether using semiring validation helps produce better errors and if the fact that Data.Semiring.Free isn't a real semiring causes any issues for the routing library; I feel like we ought to have an answer for what will happen to the routing library before going ahead with this. The easiest thing would of course be to put semiring validation and Data.Semiring.Free back inside the routing library (if you go far back enough in the git history of the routing library it appears that it originated inside there), but perhaps investigating these things will help us better understand/motivate semiring validation.

@JordanMartinez JordanMartinez added purs-0.15 A reminder to address this issue or merge this PR before we release PureScript v0.15.0 type: breaking change A change that requires a major version bump. labels Dec 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
purs-0.15 A reminder to address this issue or merge this PR before we release PureScript v0.15.0 type: breaking change A change that requires a major version bump.
Projects
None yet
Development

No branches or pull requests

4 participants