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

Collections #56

Merged
merged 12 commits into from
May 26, 2024
Merged

Collections #56

merged 12 commits into from
May 26, 2024

Conversation

4P5
Copy link
Contributor

@4P5 4P5 commented May 25, 2024

Adds the Collections API which is designed to offload bulk table operations to Java as an optimization.

I've tested all methods for functionality, but some might not handle certain cases. This PR is to allow maintainers to give feedback on the code.

Credit to @TheBunnyMan123 for the initial implementation of the API.

4P5 added 2 commits May 25, 2024 17:14
- Moved functional table/array methods into a new class
- Support multiple types for sum, difference, product, and quotient
- Fixed userdata return values instead of Vectors
- Added the functional methods map, each, flatMap, and filter
Copy link
Contributor

@TheBunnyMan123 TheBunnyMan123 left a comment

Choose a reason for hiding this comment

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

This all looks great, but could you add the new methods to the wiki directory in the root of the repository

Copy link
Collaborator

@PoolloverNathan PoolloverNathan left a comment

Choose a reason for hiding this comment

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

LGTM, need to pad a new release

@PoolloverNathan
Copy link
Collaborator

misclick sorry

@PoolloverNathan
Copy link
Collaborator

I'll do the wiki part soon™

Copy link
Collaborator

@PoolloverNathan PoolloverNathan left a comment

Choose a reason for hiding this comment

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

actually, not approving — map and friends don't have lang entries

@PoolloverNathan PoolloverNathan removed the request for review from TheBunnyMan123 May 26, 2024 18:30
Copy link
Collaborator

@PoolloverNathan PoolloverNathan left a comment

Choose a reason for hiding this comment

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

alright did a real review this time

wiki/Collections.md Outdated Show resolved Hide resolved
wiki/Collections.md Outdated Show resolved Hide resolved
wiki/Table-Math.md Outdated Show resolved Hide resolved
@PoolloverNathan PoolloverNathan dismissed their stale review May 26, 2024 19:20

applied all of my suggestions

@PoolloverNathan
Copy link
Collaborator

PoolloverNathan commented May 26, 2024

alright we've got the shit now we need to get it in, running tests now

@PoolloverNathan
Copy link
Collaborator

tests passed, let's merge

Copy link
Collaborator

@PoolloverNathan PoolloverNathan left a comment

Choose a reason for hiding this comment

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

alright now tests run

@PoolloverNathan PoolloverNathan dismissed their stale review May 26, 2024 19:53

letting Bunny make sure this actually looks good

@PoolloverNathan PoolloverNathan requested review from TheBunnyMan123 and FokoHetman and removed request for TheBunnyMan123 May 26, 2024 19:53
@FokoHetman
Copy link

under no situation I should be allowed to review stuff
Yeah. seems rather good. My knowledge of figura plugins and everything used here didn't see anything to change really.
I'll test it myself rq and.. probably approved.

Copy link

@FokoHetman FokoHetman left a comment

Choose a reason for hiding this comment

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

tested. It works well and all. Approved

@PoolloverNathan PoolloverNathan merged commit b66af44 into Figura-Goofballs:main May 26, 2024
2 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.

4 participants