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

Make isJust_ == not . isNothing_ #442

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hovind
Copy link
Contributor

@hovind hovind commented Mar 9, 2020

Right now isJust_ returns False for a RowT (Nullable (QGenExpr ctxt be s)) where

data RowT f =
  RowT
    { _uuid :: !(C f Text)
    , _foo :: !(C f (Maybe Int))
    }
  deriving anyclass (Beamable)
  deriving (Generic)

if the _foo :: !(C f (Maybe Int)) field is null, even if _uuid :: !(C f Text) is non-null.

Perhaps this is intended behaviour, but I find it confusing that a isJust_ x = False for the same x for which isNothing_ x = False.

Please let me know if the PR is missing something, if something is unclear or if I'm plain wrong. :)

@3noch 3noch added the breaking change Change will require a major version bump label Jun 10, 2020
@kmicklas
Copy link
Member

kmicklas commented Aug 9, 2020

Hmm, this is a bit confusing, but I think the current behavior makes sense and/or is at least useful since it distinguishes more values. Can we fix this with better documentation instead?

@hovind
Copy link
Contributor Author

hovind commented Aug 10, 2020

Hmm, this is a bit confusing, but I think the current behavior makes sense and/or is at least useful since it distinguishes more values. Can we fix this with better documentation instead?

Personally, I disagree that isJust_ x == False when x :: RowT (Nullable (QGenExpr ctxt be s)) has a single field that is NULL makes sense, sounds counter-intuitive to me.

As for its usefulness, I no longer have access to the code base where this issue came up, so I'm not sure if the issues we had could be solved by substituting not . isNothing_.

Documentation sounds great regardless, thanks for picking this up! 😃

@3noch
Copy link
Contributor

3noch commented Aug 10, 2020

Perhaps we should rename isJust_ to isEveryFieldJust_ or something longer and more obvious.

@tathougies
Copy link
Collaborator

This is somewhat intended behavior due to the difficulty of dealing with nullables in SQL... We can't determine if the Maybe came from the fact the row didn't exist, versus the row simply containing a NULL value. It's mentioned somewhere in the docs, but it would probably be worth it to dedicate a whole page to the gotchas with NULL in SQL. Unfortunately, the lack of sum types in SQL is a major pain.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Change will require a major version bump
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants