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

Add support for grabbing the involved event in SimplePropertyExpression #5309

Closed

Conversation

TheLimeGlass
Copy link
Collaborator

Description

Add support for grabbing the involved event in SimplePropertyExpression. Useful so that you don't have to use PropertyExpression just so that you can grab the event.

@TheLimeGlass TheLimeGlass added the feature Pull request adding a new feature. label Jan 2, 2023
Copy link
Member

@TPGamesNL TPGamesNL left a comment

Choose a reason for hiding this comment

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

I don't think SimplePropertyExpressions should have access to the Event. If it needs the Event, it should extend PropertyExpression instead. The Simple part of it means it doesn't do that much, including not using the Event.

@TheLimeGlass
Copy link
Collaborator Author

I don't think SimplePropertyExpressions should have access to the Event. If it needs the Event, it should extend PropertyExpression instead. The Simple part of it means it doesn't do that much, including not using the Event.

An example would be in my other pull request, when all I needed was simply the event #5308

@TPGamesNL
Copy link
Member

An example would be in my other pull request, when all I needed was simply the event #5308

Yea I think it's fair that that's a PropertyExpression now

@TheLimeGlass TheLimeGlass requested review from TPGamesNL and removed request for TPGamesNL January 2, 2023 18:10
@TheLimeGlass TheLimeGlass added the up for debate When the decision is yet to be debated on the issue in question label Jan 2, 2023
@TheLimeGlass
Copy link
Collaborator Author

TheLimeGlass commented Aug 7, 2023

I don't think SimplePropertyExpressions should have access to the Event. If it needs the Event, it should extend PropertyExpression instead. The Simple part of it means it doesn't do that much, including not using the Event.

I totally see the reasoning behind this, but when it comes to an event specific value like player cursor from an inventory click event; I believe there needs to be support for getting the event.

Your argument makes sense, as we don't want a SimplePropertyExpression to allow multiple expression inputs. That would defeat the whole purpose of SimplePropertyExpression vs PropertyExpression.

A solution I see, is making the init final in the SimplePropertyExpression. That way the implementing class cannot use the event for multiple expression inputs, and with this pull, you'd still be able to get an event for grabbing that value from it.

@UnderscoreTud
Copy link
Member

A solution I see, is making the init final in the SimplePropertyExpression. That way the implementing class cannot use the event for multiple expression inputs, and with this pull, you'd still be able to get an event for grabbing that value from it.

I don't believe it should be made final because you wouldn't be able to use parse tags/marks then.

@TheLimeGlass
Copy link
Collaborator Author

A solution I see, is making the init final in the SimplePropertyExpression. That way the implementing class cannot use the event for multiple expression inputs, and with this pull, you'd still be able to get an event for grabbing that value from it.

I don't believe it should be made final because you wouldn't be able to use parse tags/marks then.

Ya, but you really wouldn't need it for a simple property expression.

Could be passed into a collection field object aswell, that could also involve the event.

Copy link
Member

@Moderocky Moderocky left a comment

Choose a reason for hiding this comment

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

Are there any pending changes that require this behaviour?

@TheLimeGlass
Copy link
Collaborator Author

Closing as the underlining issue was solved by making isSingle not final for PropertyExpressions and also you should be using an EventValueExpression for event related expressions.

@TheLimeGlass TheLimeGlass deleted the feature/event-in-simple-property-expression branch August 27, 2023 04:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Pull request adding a new feature. up for debate When the decision is yet to be debated on the issue in question
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants