Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Revert "Ensure Maybe propagates error information (#411)" #440

Merged
merged 2 commits into from
Feb 19, 2021

Conversation

Tankanow
Copy link
Contributor

@Tankanow Tankanow commented Dec 7, 2020

This reverts commit da3cc96.

See description in #439. I don't think this was intended to be a breaking change, but it will be for many consumers. The extra error information is not worth breaking consumers.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 95.707% when pulling c42ecf8 on Tankanow:ISSUE-439 into 5419cff on alecthomas:master.

@Tankanow
Copy link
Contributor Author

Tankanow commented Jan 4, 2021

@alecthomas, who has write access to this repo to consider this PR?

@spacegaier
Copy link
Collaborator

Alec as the owner has, but he is not active here anymore, so it's basically only me currently.

I am not convinced about your PR, hence why I did not comment yet. My current leaning is towards: That is a nonsensical validator in the Maybe context, since the goal of a Maybe is to specifically allow None.

Based on that, you could argue it is not breaking any "valid" use cases. Also given that yours is the only feedback after 0.12.1 has been out for a month, you seem to be the only one relying on that nonsensical validator.

@Tankanow
Copy link
Contributor Author

Tankanow commented Jan 4, 2021

@spacegaier, this is definitely a valid use case. The example I used in the Issue was just a demonstration. If I'm writing my own validators, I expect them to be composable with Maybe. In other words, I shouldn't have to do a null check in my own validator if I use it with Maybe, that's what Maybe is for.

Also, regardless of my example, this is a behavioral change in the library that is also breaking for many consumers.

@Tankanow
Copy link
Contributor Author

@spacegaier, can we please revisit this issue. This is still a breaking change. Honestly, it makes the Maybe validator useless.

@alecthomas alecthomas merged commit 1720439 into alecthomas:master Feb 19, 2021
@alecthomas
Copy link
Owner

Agreed, merged.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants