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 a way to get patterns from the PropertyCondition #7107

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

Conversation

TheLimeGlass
Copy link
Collaborator

Description

Adds a way to get the property condition patterns, since I see many people making patterns similar to PropertyCondition but can't use the PropertyCondition register method due to the pattern.

This allows developers to collect and use the patterns in the way they want.

String[] patterns = Streams.concat(
		Arrays.stream(getPatterns(PropertyType.CAN, property, type)),
		Arrays.stream(getPatterns(PropertyType.IS, property, type)))
	.toArray(String[]::new);
Skript.registerCondition(condition, ConditionType.PROPERTY, patterns);

Target Minecraft Versions: any
Requirements: none
Related Issues: none

Copy link
Member

@sovdeeth sovdeeth 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 otherwise

Comment on lines 67 to 72
/**
* Indicates that the condition is in a form of <code>something is/are something</code>,
* also possibly in the negated form
*/
IS,

Copy link
Member

Choose a reason for hiding this comment

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

this is just BE, why does it need to be added

Copy link
Collaborator Author

@TheLimeGlass TheLimeGlass Sep 21, 2024

Choose a reason for hiding this comment

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

BE doesn't equal is/are. There is no reference to BE anywhere. Nico realized this after the PR.

Copy link
Member

Choose a reason for hiding this comment

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

well is/are are conjugated forms of "to be", so BE makes sense

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is this blocking to not have both enums?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, either rename BE or keep BE (i prefer keeping)
It's just confusing to have two types that do and mean the exact same thing

Copy link
Member

Choose a reason for hiding this comment

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

renaming would be a breaking change!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not renaming, it's adding the proper terminology.

Copy link
Member

Choose a reason for hiding this comment

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

As already mentioned, BE is the proper terminology. No need to add another that does the same thing.

@sovdeeth sovdeeth added the enhancement Feature request, an issue about something that could be improved, or a PR improving something. label Sep 21, 2024
Comment on lines 67 to 72
/**
* Indicates that the condition is in a form of <code>something is/are something</code>,
* also possibly in the negated form
*/
IS,

Copy link
Member

Choose a reason for hiding this comment

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

As already mentioned, BE is the proper terminology. No need to add another that does the same thing.

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants