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.

Sign up for GitHub

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

Jump to bottom

data: Rename Maybe.Defined/Empty to Present/Absent and export them to the kyo package #734

Merged
merged 6 commits into from
Oct 15, 2024
Merged

data: Rename Maybe.Defined/Empty to Present/Absent and export them to the kyo package #734

merged 6 commits into from
Oct 15, 2024

Conversation

fwbrasil
Copy link
Collaborator

@fwbrasil fwbrasil commented Oct 9, 2024

I sometimes avoid pattern matching on Maybe because it feels too verbose. This PR exports Maybe.Empty and Maybe.Defined as members in the kyo package. It also reduces the syntax overhead for tracking Abort[Empty]. It seems reasonable since the type names shouldn't conflict with common types.

@fwbrasil fwbrasil force-pushed the export-maybe-empty-defined branch from 947391a to ddc9ebc Compare October 11, 2024 22:48
@hearnadam
Copy link
Collaborator

As discussed offline, I am concerned with collisions with Empty. We discussed a few potential options,

  • Present/Absent - nice parity
  • Just/Void - Haskell like

Or perhaps we could just export the Defined... leaving underscore pattern matches where necessary...

@johnhungerford
Copy link
Contributor

+1 Present/Absent, although frankly I don't think ever seen anyone actually use scala streams or therefore the std Empty object

@fwbrasil
Copy link
Collaborator Author

I like Present/Absent as well since it'd hardly conflict with other types and feels more natural to communicate meaning to newcomers. Just/Void has the Haskell precedence that helps with understanding but I find it a bit opaque.

@fwbrasil fwbrasil force-pushed the export-maybe-empty-defined branch from ddc9ebc to 41688c5 Compare October 14, 2024 19:06
@fwbrasil
Copy link
Collaborator Author

I've updated the PR with the rename to Present/Absent to see how it feels. It seems ok, the only thing I wasn't sure about is if we should also rename methods like Maybe.empty to Maybe.absent. I think not since people will instinctively search for methods like empty, isDefined, etc and they still seem to make sense even with the new naming.

hearnadam
hearnadam reviewed Oct 14, 2024
View reviewed changes
kyo-prelude/shared/src/main/scala/kyo/internal/LayerMacros.scala Outdated
@@ -114,7 +114,7 @@ private[kyo] object LayerMacros:
case And(left: LayerLike[A], right: LayerLike[A])
case To(left: LayerLike[A], right: LayerLike[A])
case Value(value: A)
case Empty
case Absent
Copy link
Collaborator

Choose a reason for hiding this comment

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

Accidental?

hearnadam
hearnadam approved these changes Oct 15, 2024
View reviewed changes
Copy link
Collaborator

@hearnadam hearnadam left a comment

Choose a reason for hiding this comment

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

Perhaps worth renaming PR title for release notes - LGTM!

@fwbrasil fwbrasil changed the title data: export Maybe.Empty and Maybe.Defined to the kyo package data: Rename Maybe.Defined/Empty to Present/Absent and export them to the kyo package Oct 15, 2024
@fwbrasil fwbrasil merged commit 354267d into main Oct 15, 2024
3 checks passed
@fwbrasil fwbrasil deleted the export-maybe-empty-defined branch October 15, 2024 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hearnadam hearnadam hearnadam approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@fwbrasil @hearnadam @johnhungerford