Skip to content

Java function rework #7969

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

Open
wants to merge 37 commits into
base: dev/feature
Choose a base branch
from
Open

Conversation

Efnilite
Copy link
Member

@Efnilite Efnilite commented Jun 24, 2025

Problem

In preparation for named function arguments, which allows changing the order of appearance of function arguments, a replacement has to be created for JavaFunctions. JavaFunctions use indices to get function arguments, which is incompatible with the planned design for functions.

Other than that, registering functions is just really ugly. This PR improves that.

Old
image

New
image

This is a part of the larger function rework.

Solution

A DefaultFunction has several advantages over a JavaFunction.

  • DefaultFunction has a much more modern function builder, resulting in nicer looking code.
  • DefaultFunction utilizes Java types to specify exactly what arguments to expect. When defining a parameter, instead of having to use a ClassInfo and then specifying that it accepts multiple inputs separately, you can now simply write String[].class.
  • DefaultFunction removes some confusing behaviour from JavaFunctions. For example, in the location function, the amount of worlds would sometimes be more than one. This is bad design, as the world parameter is defined as being single. DefaultFunction now correctly selects a single world.
  • DefaultFunction now allows you to get arguments by their name, instead of their index, which results in clearer code.
  • DefaultFunction has no children, reducing the inheritance as seen in (Simple)JavaFunction.
  • DefaultFunction supports String arrays in its since method.

Testing Completed

  • Added JUnit tests
  • Converted some high usage JavaFunctions to DefaultFunctions, and made sure that their tests run

Supporting Information

  • Deprecates JavaFunction, SimpleJavaFunction.
  • Updates documentation classes to use Function instead of JavaFunction

Completes: none
Related: #7590 (since array support)

@Efnilite Efnilite added the bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. label Jun 24, 2025
@Efnilite Efnilite requested review from a team as code owners June 24, 2025 23:42
@Efnilite Efnilite added feature Pull request adding a new feature. functions Related to functions labels Jun 24, 2025
@Efnilite Efnilite requested review from UnderscoreTud and removed request for a team June 24, 2025 23:42
Copy link
Member

@UnderscoreTud UnderscoreTud left a comment

Choose a reason for hiding this comment

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

This is a huge improvement over the old functions, good job!!

@skriptlang-automation skriptlang-automation bot added the needs reviews A PR that needs additional reviews label Jun 25, 2025
@Efnilite Efnilite requested a review from UnderscoreTud June 25, 2025 10:05
@skriptlang-automation skriptlang-automation bot removed the needs reviews A PR that needs additional reviews label Jun 25, 2025
@Efnilite Efnilite requested a review from Absolutionism June 26, 2025 00:21
Efnilite and others added 4 commits June 26, 2025 02:43
Co-authored-by: SirSmurfy2 <82696841+Absolutionism@users.noreply.github.com>
@Efnilite Efnilite requested a review from Absolutionism June 26, 2025 01:03
Copy link
Contributor

@Absolutionism Absolutionism left a comment

Choose a reason for hiding this comment

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

Looks good, I like the change.

Copy link
Member

@APickledWalrus APickledWalrus left a comment

Choose a reason for hiding this comment

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

Just marking that I need to review this again

@skriptlang-automation skriptlang-automation bot removed the feature-ready A PR/issue that has been approved, tested and can be merged/closed in the next feature version. label Jul 2, 2025
@Efnilite Efnilite requested a review from APickledWalrus July 2, 2025 21:23
Copy link
Member

@APickledWalrus APickledWalrus left a comment

Choose a reason for hiding this comment

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

looks pretty good. almost ready

Co-authored-by: sovdee <10354869+sovdeeth@users.noreply.github.com>
@Efnilite Efnilite requested a review from sovdeeth July 7, 2025 20:23
@sovdeeth sovdeeth moved this to In Review in 2.13 Releases Jul 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. feature Pull request adding a new feature. functions Related to functions
Projects
Status: In Review
Development

Successfully merging this pull request may close these issues.

6 participants