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

Make Parser#canParse abstract #7099

Draft
wants to merge 4 commits into
base: dev/feature
Choose a base branch
from

Conversation

Fusezion
Copy link
Contributor

@Fusezion Fusezion commented Sep 20, 2024

Description

PR is a draft to act more as a proposal and wanting feedback on it first before making ready to review. I'm aware of the implications behind this being merge for addons.

This PR aims to complete one of my older issues, the intended design changed a bit, due to breaking everything implemented without defining canParse and as a means to better handle this in the future.

Before Sovde ask if there is any benefit to changing this, my response is yes, not only was it impossible to omit the usage of .parse() (or the canParse part) due to it previously returning true by default and spamming the server with UnsupportedOperationExceptionm but is also a core part of the parser class and should be handled as one.

This is a breaking change for addons that did the same thing as skript and rely on the default implementation of true, however the change itself is very light and most addons could easily add the method without breaking anything else, if someone has a better implementation that doesn't break old addons that were abandoned or discontinued I'm willing to hear ideas. Although I would prefer to, not see a repeat of Timespan#fromTicks_i()


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

@sovdeeth
Copy link
Member

sovdeeth commented Sep 20, 2024

I'd rather see the default simply change to false than be abstract tbh

@Fusezion
Copy link
Contributor Author

Fusezion commented Sep 20, 2024

I'd rather see the default simply change to false rather than be abstract tbh

I'd like a few more opinions on this before I decide it, but from my perspective I much rather force something to exist and acknowledge the fact it can or cannot parse vs. defaulting to either and leaving it up to interpretation.

In addition I believe this path makes it much easier for developers to fix these issues before they make a release on their addons. Afterall even skript had 30+ missing canParse who knows how bad other addons are. This decision was made mostly with that in mind.

@sovdeeth
Copy link
Member

That's also a good point

@Pikachu920
Copy link
Member

we could add a warning to the default impl for a few versions first

@sovdeeth sovdeeth added the enhancement Feature request, an issue about something that could be improved, or a PR improving something. label Sep 21, 2024
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