Skip to content

ExprSkull - Breaking Change #7904

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 6 commits into
base: dev/feature
Choose a base branch
from

Conversation

Absolutionism
Copy link
Contributor

@Absolutionism Absolutionism commented May 30, 2025

Problem

ExprEyeLocation and ExprSkull has a pattern confliction of head of ... which could lead to the retrieval of the wrong expression depending on the use case.

Solution

This PR fixes this confliction by removing the option of head in ExprSkull
Thus allowing users to be able to retrieve the exact expression/return type they desire.
Is a breaking change.

Testing Completed

N/A

Supporting Information

N/A


Completes: #7837
Related: none

@Absolutionism Absolutionism requested a review from a team as a code owner May 30, 2025 20:02
@Absolutionism Absolutionism requested review from Efnilite and cheeezburga and removed request for a team May 30, 2025 20:02
@skriptlang-automation skriptlang-automation bot added the needs reviews A PR that needs additional reviews label May 30, 2025
@sovdeeth
Copy link
Member

this seems a bit more breaking than is really necessary
player's eyes works just fine without location
Can I ask why you chose to change this expression instead of, for example, limiting the skull expression to just player's skull?

@sovdeeth sovdeeth added the breaking changes Pull or feature requests that contain breaking changes (API, syntax, etc.) label May 30, 2025
@Absolutionism
Copy link
Contributor Author

this seems a bit more breaking than is really necessary player's eyes works just fine without location Can I ask why you chose to change this expression instead of, for example, limiting the skull expression to just player's skull?

Well the lack of discussion in the issue didn't really point towards changing ExprSkull as the only opinion that was made was to require location by @EquipableMC

@sovdeeth
Copy link
Member

Gotcha
Do you have any opinions on it? I personally would prefer having the skull expression changed instead, but I'm also pretty sure that would impact more users so it might be preferable to keep it as is

@Absolutionism
Copy link
Contributor Author

Do you have any opinions on it?

I'm neutral, either or.

@Fusezion
Copy link
Contributor

Being the one who made the issue I'll at least give my perspective. As I said on the issue either or I personally push towards ExprSkull more than ExprEyeLocation, but I also know I see head of player more than skull of player

@sovdeeth
Copy link
Member

Moving this to dev/feature due to breaking changes

@sovdeeth sovdeeth changed the base branch from dev/patch to dev/feature May 30, 2025 22:09
@cheeezburga
Copy link
Member

I don't think I love making the location part here mandatory. Would rather see the change made to ExprSkull - even if it is more breaking right now, I think it's better in the long run.

@sovdeeth sovdeeth added the enhancement Feature request, an issue about something that could be improved, or a PR improving something. label Jun 3, 2025
@APickledWalrus APickledWalrus linked an issue Jun 4, 2025 that may be closed by this pull request
1 task
@Absolutionism
Copy link
Contributor Author

So the current count seems to be
Change ExprEyeLocation - Equip (1)
Change ExprSkull - Fusezion, Sovde, and Cheez (3)

So I'll change this PR to change ExprSkull

@Absolutionism Absolutionism changed the title ExprEyeLocation - Breaking Change ExprSkull - Breaking Change Jun 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking changes Pull or feature requests that contain breaking changes (API, syntax, etc.) enhancement Feature request, an issue about something that could be improved, or a PR improving something. needs reviews A PR that needs additional reviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Conflict with ExprSkull and ExprEyeLocation
4 participants