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

Slight Exception message changes #7116

Open
wants to merge 6 commits into
base: dev/patch
Choose a base branch
from

Conversation

Asleeepp
Copy link
Contributor

@Asleeepp Asleeepp commented Sep 27, 2024

Description

Honestly, I'm not really the biggest fan of how the exception is laid out, especially lines 1759-1763, and the constant "You'll be given instructions on how to report this", but I'd rather not change that.
This PR aims to fix some punctation errors I found, and also reword some sentences.


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

@sovdeeth sovdeeth added the enhancement Feature request, an issue about something that could be improved, or a PR improving something. label Sep 27, 2024
src/main/java/ch/njol/skript/Skript.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/Skript.java Show resolved Hide resolved
src/main/java/ch/njol/skript/Skript.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/Skript.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/Skript.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/Skript.java Outdated Show resolved Hide resolved
@Asleeepp Asleeepp requested a review from sovdeeth October 3, 2024 21:54
Copy link
Member

@APickledWalrus APickledWalrus left a comment

Choose a reason for hiding this comment

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

Some brief thoughts. I like the idea here

String issuesUrl = "https://github.com/SkriptLang/Skript/issues";
String downloadUrl = "https://github.com/SkriptLang/Skript/releases/latest";
Copy link
Member

Choose a reason for hiding this comment

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

It might be better to grab this from the update checker

Though I don't think the update checker currently records this for GitHub, so a TODO comment would suffice for now

Copy link
Contributor Author

@Asleeepp Asleeepp Oct 9, 2024

Choose a reason for hiding this comment

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

Wouldn't using releases/latest (like it is currently) suffice for this? I don't see the difference it would make.

Copy link
Member

Choose a reason for hiding this comment

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

I just mean that the update checker might not always point to GitHub (e.g. in the future it could recommend Spigot if that's where you downloaded Skript from)

Comment on lines +1714 to +1716
pluginPackages.entrySet().stream()
.filter(entry -> element.getClassName().startsWith(entry.getKey()))
.forEach(entry -> stackPlugins.add(entry.getValue()));
Copy link
Member

Choose a reason for hiding this comment

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

You could just compile a set of plugins based on which ones provided the classes using https://hub.spigotmc.org/javadocs/spigot/org/bukkit/plugin/java/JavaPlugin.html#getProvidingPlugin(java.lang.Class)
Though that would require obtaining a Class instance, which might be a little heavy, but it is certainly worth trying IMO

logEx();
}

private static String getStatusDescription(ReleaseStatus status) {
Copy link
Member

Choose a reason for hiding this comment

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

It might be worth integrating this into the ReleaseStatus enum

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.

6 participants