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

Clean list API #7290

Merged
merged 7 commits into from
Feb 17, 2025
Merged

Clean list API #7290

merged 7 commits into from
Feb 17, 2025

Conversation

tsnobip
Copy link
Contributor

@tsnobip tsnobip commented Feb 11, 2025 •
edited
Loading

  • remove List.t
  • rename toShuffled to shuffle and deprecate it
  • deprecate List.*Assoc functions

I think this solves most issues raised in rescript-lang/rescript-core/issues/192.

I have no good idea for List.concatMany so I just kept it.

I'd rather keep List.has and List.length as it is today and maybe we could add Array.has for consistency.

@tsnobip tsnobip requested a review from cknitt February 11, 2025 16:25
github-actions[bot]
github-actions bot reviewed Feb 11, 2025
View reviewed changes
Copy link

@github-actions github-actions bot left a comment •
edited
Loading

Choose a reason for hiding this comment

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

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Syntax Benchmarks'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.05.

Benchmark suite Current: c20e7b1 Previous: e1b7fb7 Ratio
Parse RedBlackTree.res - time/run 1.34950402 ms 1.2123143266666667 ms 1.11
Parse Napkinscript.res - time/run 42.166715106666665 ms 39.28006235333333 ms 1.07
Parse HeroGraphic.res - time/run 5.705078513333333 ms 5.13472718 ms 1.11

This comment was automatically generated by workflow using github-action-benchmark.

@tsnobip tsnobip changed the title Clean list api (fix #192) Clean list api (fix rescript-lang/rescript-core/issues/192) Feb 11, 2025
@tsnobip tsnobip changed the title Clean list api (fix rescript-lang/rescript-core/issues/192) Clean list API Feb 11, 2025
cknitt
cknitt reviewed Feb 12, 2025
View reviewed changes
Copy link
Member

@cknitt cknitt left a comment

Choose a reason for hiding this comment

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

Thanks a lot!

runtime/List.res Outdated
let len = length(x)
let arr = A.makeUninitializedUnsafe(len)
fillAux(arr, 0, x)
arr
}

let toShuffled = xs => {
let shuffle = xs => {
Copy link
Member

Choose a reason for hiding this comment

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

Not sure about this one.

On the one hand, it may be confusing that in the Array module shuffle mutates whereas in the List module it does not. On the other hand, we know that list is immutable...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah I'm a bit torn on this one too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what should we do about this one @cknitt?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm... I guess either is fine with me. Any preference from your side @zth?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

my personal preference would be to use shuffle given list is immutable and the signature is clear, but up to you guys!

cknitt
cknitt reviewed Feb 12, 2025
View reviewed changes
@tsnobip tsnobip requested a review from cknitt February 12, 2025 21:34
@tsnobip tsnobip force-pushed the clean-list-api branch 2 times, most recently from f89dec9 to 4d092f0 Compare February 13, 2025 08:58
@tsnobip tsnobip marked this pull request as draft February 13, 2025 08:59
@tsnobip tsnobip marked this pull request as ready for review February 13, 2025 09:23
@cknitt
Copy link
Member

cknitt commented Feb 16, 2025

@tsnobip Thanks! Holding off merging as @zth and I are currently working on #7285 and we'll get conflicts there.
Would ask you to rebase after #7285 is merged.

@cknitt
Copy link
Member

cknitt commented Feb 17, 2025

@tsnobip #7285 is merged!

@tsnobip
Copy link
Contributor Author

tsnobip commented Feb 17, 2025

@cknitt rebased!

@cknitt
Copy link
Member

cknitt commented Feb 17, 2025

Great, thanks! However, could you put the re-adding of type t for the other stdlib modules into a separate PR? As I would squash merge this one, and then we can't separate the list changes from the type t changes anymore.

@tsnobip
Copy link
Contributor Author

tsnobip commented Feb 17, 2025

@cknitt done

@cknitt
Copy link
Member

cknitt commented Feb 17, 2025

@tsnobip Completion tests are failing again now

cknitt
cknitt approved these changes Feb 17, 2025
View reviewed changes
Copy link
Member

@cknitt cknitt left a comment

Choose a reason for hiding this comment

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

Thanks again!

@cknitt cknitt merged commit d1c9aef into master Feb 17, 2025
20 checks passed
@tsnobip tsnobip deleted the clean-list-api branch February 17, 2025 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@github-actions github-actions[bot] github-actions[bot] left review comments

@cknitt cknitt cknitt 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.
2 participants
@tsnobip @cknitt