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

feat(corelib): add map to Option #6929

Merged
merged 2 commits into from
Dec 26, 2024
Merged

Conversation

cairolover
Copy link
Contributor

@cairolover cairolover commented Dec 25, 2024

Adds map to OptionTrait for more functional programming features.

moved ok_or to be close to map, regrouping functions by concern (transforming contained values).

adds missing test for ok_or.

@reviewable-StarkWare
Copy link

This change is Reviewable

@julio4
Copy link
Contributor

julio4 commented Dec 25, 2024

@cairolover Maybe we should coordinate to not work on the same things cf #6927 #6928 #6930

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @cairolover)


corelib/src/option.cairo line 179 at r1 (raw file):

    /// Transforms the `Option<T>` into a `Result<T, E>`, mapping `Option::Some(v)` to
    /// `Result::Ok(v)` and `Option::None` to `Result::Err(err)`.

why move it?

Copy link
Contributor Author

@cairolover cairolover left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @orizi)


corelib/src/option.cairo line 179 at r1 (raw file):

moved ok_or to be close to map, regrouping functions by concern (transforming contained values).

As we're adding methods to the corelib, I think it's good that we ensure that they're grouped by concern. In that case, ok_or is should be grouped with map.

I would even go as far as moving unwrap with unwrap_or, unwrap_or_default and unwrap_or_else, but that can be for another PR if you agree. It's minor stuff but makes the difference at the end.

Also: if you prefer that I make 1 change per PR instead of adding scout commits, please let me know 😁

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @cairolover)


corelib/src/option.cairo line 179 at r1 (raw file):

Previously, cairolover (cairolover) wrote…

moved ok_or to be close to map, regrouping functions by concern (transforming contained values).

As we're adding methods to the corelib, I think it's good that we ensure that they're grouped by concern. In that case, ok_or is should be grouped with map.

I would even go as far as moving unwrap with unwrap_or, unwrap_or_default and unwrap_or_else, but that can be for another PR if you agree. It's minor stuff but makes the difference at the end.

Also: if you prefer that I make 1 change per PR instead of adding scout commits, please let me know 😁

reordering in a different PR would be appreciated - this is more difficult to review this way.

thanks!

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @cairolover)


a discussion (no related file):
@TomerStarkware for 2nd eye.

Copy link
Contributor Author

@cairolover cairolover left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @orizi)


corelib/src/option.cairo line 179 at r1 (raw file):

Previously, orizi wrote…

reordering in a different PR would be appreciated - this is more difficult to review this way.

thanks!

Done.

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @cairolover)


corelib/src/option.cairo line 265 at r3 (raw file):

    /////////////////////////////////////////////////////////////////////////
    // Transforming contained values
    /////////////////////////////////////////////////////////////////////////

@enitrat should we have such headlines?

Code quote:

    /////////////////////////////////////////////////////////////////////////
    // Transforming contained values
    /////////////////////////////////////////////////////////////////////////

Copy link
Contributor

@enitrat enitrat left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @orizi)


corelib/src/option.cairo line 265 at r3 (raw file):

Previously, orizi wrote…

@enitrat should we have such headlines?

This has no impact on the generated doc site, but helps when navigating the source code. I think we could.

Copy link
Contributor

@enitrat enitrat left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @orizi)


corelib/src/option.cairo line 265 at r3 (raw file):

Previously, enitrat (Mathieu) wrote…

This has no impact on the generated doc site, but helps when navigating the source code. I think we could.

Rust has this

Copy link
Collaborator

@TomerStarkware TomerStarkware left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @enitrat and @orizi)

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 2 files at r1, 1 of 1 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @cairolover)

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @cairolover)

@orizi orizi enabled auto-merge December 26, 2024 11:46
Copy link
Collaborator

@TomerStarkware TomerStarkware left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @cairolover)

@orizi orizi added this pull request to the merge queue Dec 26, 2024
Merged via the queue into starkware-libs:main with commit a240fd0 Dec 26, 2024
47 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants