-
-
Notifications
You must be signed in to change notification settings - Fork 372
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
SoundUtils - handle Sound class for enum/interface #7199
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think you can rebase this to patch since this is a bug that should be fixed
@@ -240,4 +239,27 @@ public String toString(@Nullable Event event, boolean debug) { | |||
return builder.toString(); | |||
} | |||
|
|||
@SuppressWarnings({"deprecation", "unchecked", "rawtypes"}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be per statement. you should use the noinspection comment (alt enter > arrow down (suppress) > suppress for statement)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see the point of this. Why remove 1 line to create 3+ more lines?
And since when did SkriptLang do this? I dont think ive ever done this nor ever seen it, nor is it in the code conventions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think using suppresswarnings on an entire method may lead to confusion as to when something generally considered not good is used (like casting raw types etc). using these comments just emphasizes where they actually occur
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So is this a SkriptLang preference, or personal preference?
I personally think having a suppression for a block makes more sense than adding a suppression for every line in the block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's something we agreed on back in august or so. If it's not in the conventions yet it probably needs to be added. Warning suppression should be as specific as possible to avoid suppressing other warnings that weren't originally intended (see suppressing one deprecated method, but then a second gets deprecated too, it's good to still see the warning for the second.)
Of course, if the warnings are routine and/or the block is very small, like many init()
methods, it's not worth suppressing each line individually.
In this case I can see it either way, I don't think it matters much, since the method's quite small.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's not in the conventions yet it probably needs to be added.
It is :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Efnilite so much for SkriptLang being less strict. All those changes just seem redundant. |
i think it's to ensure a consistent style in the entire codebase. i don't make the rules, i just follow them :) |
https://github.com/SkriptLang/Skript/blob/master/code-conventions.md |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great
Description
This PR aims to fix an issue with Bukkit changing the Sound class from Enum to Interface.
This is my take on it, not sure if this is the best way to do it.
Note:
Target Minecraft Versions: any
Requirements: none
Related Issues: none