Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

Adds Option for codeintel#63637

Merged
kritzcreek merged 4 commits into
mainfrom
christoph/option
Jul 4, 2024
Merged

Adds Option for codeintel#63637
kritzcreek merged 4 commits into
mainfrom
christoph/option

Conversation

@kritzcreek

@kritzcreek kritzcreek commented Jul 4, 2024

Copy link
Copy Markdown
Contributor

Adds an Option[A any] type for codeintel

Test plan

Added Unit and PB tests

@cla-bot cla-bot Bot added the cla-signed label Jul 4, 2024
@github-actions github-actions Bot added team/graph Graph Team (previously Code Intel/Language Tools/Language Platform) team/product-platform labels Jul 4, 2024
Comment on lines +41 to +77
func (o Option[A]) Unwrap() A {
val, ok := o.Get()
if !ok {
panic("called Option.Unwrap on None")
}
return val
}

func (o Option[A]) UnwrapOr(defaultValue A) A {
val, ok := o.Get()
if !ok {
return defaultValue
}
return val
}

func (o Option[A]) UnwrapOrElse(defaultValue func() A) A {
val, ok := o.Get()
if !ok {
return defaultValue()
}
return val
}

func (o Option[A]) Or(other Option[A]) Option[A] {
if o.IsSome() {
return o
}
return other
}

func (o Option[A]) OrElse(other func() Option[A]) Option[A] {
if o.IsSome() {
return o
}
return other()
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

(non-blocking suggestion) API-wise, I would rather introduce methods outside the "basic" set on an as-needed basis rather than right away. For example, I don't think OrElse is going to be that readable at usage sites because anonymous functions cannot have their types inferred

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

What would you consider the "basic" set? I personally think the lazy getter variants are required in a strict language, but I could be convinced that Or and OrElse don't need to be here from the start.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm with Varun on this one - ergonomics of Go's generics and lack of true lazy values probably rules out Or and OrElse being used often

@varungandhi-src varungandhi-src Jul 4, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The "basic" set would probably be: Get() -> (T, bool), Set(T) -> void, Unwrap() -> T, IsSome() -> bool, IsNone() -> bool, Equals(Option[T]) -> bool and Compare(Option[T]) -> int. We can always add stuff later.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

re: Equals and Compare. I don't think you can define these guys, because you can't add a comparable constraint on a method.
How would you expect Set to behave on a None?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Actually... I'd disagree that any implementation of Set would be a good idea. Options whole point here is to let us make it clear when a *T is about perf/mutability (same thing imo) instead of optionality.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

How would you expect Set to behave on a None?

It would mutate the underlying value. Rust has this too, but it's called insert. https://doc.rust-lang.org/std/option/enum.Option.html#method.insert

@kritzcreek kritzcreek enabled auto-merge (squash) July 4, 2024 10:47
@kritzcreek kritzcreek merged commit fd991fd into main Jul 4, 2024
@kritzcreek kritzcreek deleted the christoph/option branch July 4, 2024 10:56
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla-signed team/graph Graph Team (previously Code Intel/Language Tools/Language Platform) team/product-platform

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants