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

ExpressionPropertyCondition #7612

Open
1 task done
TheAbsolutionism opened this issue Feb 14, 2025 · 6 comments
Open
1 task done

ExpressionPropertyCondition #7612

TheAbsolutionism opened this issue Feb 14, 2025 · 6 comments
Labels
enhancement Feature request, an issue about something that could be improved, or a PR improving something. up for debate When the decision is yet to be debated on the issue in question

Comments

@TheAbsolutionism
Copy link
Contributor

Suggestion

Add an ExpressionPropertyCondition class (or different name)
Allowing to quickly register a condition that utilizes property of a type/object/expression and the property for the condition.
register(Example.class, PropertyType.BE, "visible", "custom name[s]", "entities") or whichever order.
Resulting in patterns of

"%entities%'[s] custom name[s] (is|are) visible",
"%entities%'[s] custom name[s] (isn't|is not|are not|aren't) visible",
"custom name[s] of %entities% (is|are) visible",
"custom name[s] of %entities% (isn't|is not|are not|aren't) visible"

Why?

Useful

Other

No response

Agreement

  • I have read the guidelines above and affirm I am following them with this suggestion.
@Pikachu920 Pikachu920 added enhancement Feature request, an issue about something that could be improved, or a PR improving something. up for debate When the decision is yet to be debated on the issue in question labels Feb 15, 2025
@Pikachu920
Copy link
Member

Thanks for the issue. I don't think I see the value in this over registering a propertycondition using Skript#registerCondition (or registry api).

@TheAbsolutionism
Copy link
Contributor Author

@Pikachu920
Thanks for your response. Could you perhaps explain a bit more on your personal opinion of it not having much value?
The intention of this suggestion is of the same intentions on why PropertyCondition, PropertyExpression and SimplePropertyExpression exist. Just another shortcut and QOL class.
Though currently it won't replace too many existing conditions, it would be beneficial to the active SkriptLang team members and contributors that do make PRs. Additionally for the addon developers that deal with conditions and could possibly benefit from its addition.

@Pikachu920
Copy link
Member

It seems like the shortcut here is avoiding writing the patterns manually. I think I'd rather see these classes manually write the patterns rather than extend a special class. Even if we want to avoid writing the patterns, that can be done by adding a method to get the patterns from the property and expression.

I also feel it is undesirable to have Expression in the class name when this isn't really adding an expression.

@sovdeeth
Copy link
Member

I agree, this seems like a lot for what could be a simple static helper method.

@TheAbsolutionism
Copy link
Contributor Author

I also feel it is undesirable to have Expression in the class name when this isn't really adding an expression.

Well, I didn't have a good name for it, that's why I had put (or different name) in the original post.

I agree, this seems like a lot for what could be a simple static helper method.

That is a good alternative, I'm fine with just adding a new method within PropertyCondition

@Pikachu920
Copy link
Member

I'd really rather not even have the method. If I was reading this code, I'd prefer to see this pattern written out instead of using a helper method. It's easy to understand with something very simple like x is y or x isn't y, but the method proposed here feels significantly harder to reason about.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature request, an issue about something that could be improved, or a PR improving something. up for debate When the decision is yet to be debated on the issue in question
Projects
None yet
Development

No branches or pull requests

3 participants