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

Generate JSON docs with Gson #5738

Merged
merged 30 commits into from
Sep 19, 2024
Merged

Generate JSON docs with Gson #5738

merged 30 commits into from
Sep 19, 2024

Conversation

Pikachu920
Copy link
Member

@Pikachu920 Pikachu920 commented Jun 10, 2023

Description

This PR switches the JSON doc generation to Gson (which is included w/ spigot). The current generation method (I think it's just some string replacing) often generates invalid JSON. Switching to Gson should help prevent this as Gson will deal with the JSON generation, while we just provide the data.

You can see the generated JSON here: https://docs.skriptlang.org/nightly/fix/json-docs/docs.json


Target Minecraft Versions: N/A
Requirements: N/A
Related Issues: SkriptLang/skript-docs#31

@Pikachu920 Pikachu920 requested a review from AyhamAl-Ali June 11, 2023 00:01
@Pikachu920 Pikachu920 added bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. documentation Related to Skript's official documentation. labels Jun 11, 2023
@Pikachu920 Pikachu920 marked this pull request as ready for review June 11, 2023 05:32
Copy link
Member

@AyhamAl-Ali AyhamAl-Ali left a comment

Choose a reason for hiding this comment

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

Pikachu is killing it again 🚀

Amazing changes! This is a quick review I didn't dig deep or test anything yet but looks pretty clean.

src/main/java/ch/njol/skript/doc/JSONGenerator.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/doc/HTMLGenerator.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/lang/function/Signature.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/doc/JSONGenerator.java Outdated Show resolved Hide resolved
@TheLimeGlass TheLimeGlass self-requested a review June 19, 2023 01:49
@Pikachu920 Pikachu920 marked this pull request as draft August 30, 2023 01:28
@Moderocky Moderocky force-pushed the master branch 3 times, most recently from bd134d0 to 3f08853 Compare September 16, 2023 16:59
@Pikachu920 Pikachu920 changed the base branch from master to dev/patch December 19, 2023 03:27
@Pikachu920 Pikachu920 changed the base branch from dev/patch to dev/feature December 19, 2023 03:43
@Pikachu920 Pikachu920 marked this pull request as ready for review December 19, 2023 05:18
@Romitou
Copy link
Member

Romitou commented May 9, 2024

By default, GSON escapes strings containing HTML content. This is the intended behaviour, but for some reason the escaping occurs several times, as can be seen in the JSON for some examples.

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.

remove licenses and add javadocs

# Conflicts:
#	src/main/java/ch/njol/skript/doc/HTMLGenerator.java
#	src/main/java/ch/njol/skript/lang/function/Signature.java
# Conflicts:
#	src/main/java/ch/njol/skript/doc/HTMLGenerator.java
#	src/main/java/ch/njol/skript/lang/function/Signature.java
@Pikachu920 Pikachu920 linked an issue Sep 6, 2024 that may be closed by this pull request
Copy link
Member

@Efnilite Efnilite left a comment

Choose a reason for hiding this comment

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

in HTMLGenerator you could use File(parent, child) to avoid manually having to add the / or using File.separator, or you could use what you have rn but you should make it more consistent because it varies.

you can also maybe add tests for the JSONGenerator

besides that very cool

@SuppressWarnings("unchecked")
public void generate() {
for (File f : template.listFiles()) {
for (File f : templateDir.listFiles()) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
for (File f : templateDir.listFiles()) {
for (File file : templateDir.listFiles()) {

if (f.getName().matches("css|js|assets")) { // Copy CSS/JS/Assets folders
String slashName = "/" + f.getName();
File fileTo = new File(output + slashName);
File fileTo = new File(outputDir + slashName);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
File fileTo = new File(outputDir + slashName);
File fileTo = new File(outputDir, file.getName());

doesn't this work? this way you don't need slashName

src/main/java/ch/njol/skript/doc/JSONGenerator.java Outdated Show resolved Hide resolved
Pikachu920 and others added 3 commits September 9, 2024 10:55
Co-authored-by: _tud <98935832+UnderscoreTud@users.noreply.github.com>
Copy link
Member

@Romitou Romitou left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines +43 to +44
private static <T> int calculateCollisionCount(Iterator<? extends T> potentialCollisions, Predicate<T> collisionCriteria,
Predicate<T> equalsCriteria) {
Copy link
Member

Choose a reason for hiding this comment

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

What's this indentation doing?

@Pikachu920 Pikachu920 dismissed stale reviews from APickledWalrus and AyhamAl-Ali September 19, 2024 00:33

addressed

@Pikachu920 Pikachu920 merged commit 30ab7d3 into dev/feature Sep 19, 2024
5 checks passed
@APickledWalrus APickledWalrus deleted the fix/json-docs branch October 13, 2024 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. documentation Related to Skript's official documentation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🐛 docs.json bugs
8 participants