Skip to content

Enforce documentation for configs via test suites - #293

Open
Toffikk wants to merge 16 commits into
mainfrom
dev/require-docs-for-configs
Open

Enforce documentation for configs via test suites#293
Toffikk wants to merge 16 commits into
mainfrom
dev/require-docs-for-configs

Conversation

@Toffikk

@Toffikk Toffikk commented Jul 15, 2026

Copy link
Copy Markdown
Member

This pull request adds tests for the global and world configuration classes which enforce all options have documentation attached to them.
The verification can be opted out of by annotating the relevant config with @Undocumented along with a reason.

In addition to above it completes https://panel.canvasmc.io/dashboard/tasks/cmr0gyjxp00ap0klpfncznk15


Example output

image image

@Toffikk
Toffikk requested a review from a team July 15, 2026 17:48
@Toffikk Toffikk self-assigned this Jul 15, 2026
@Toffikk
Toffikk requested review from R00tB33rMan and removed request for a team July 15, 2026 17:48
@Toffikk Toffikk added enhancement New feature or request priority: medium status: waiting for review Waiting for review from an official member of the team labels Jul 15, 2026
@Toffikk
Toffikk requested review from Dueris and devnootnoot July 15, 2026 17:49
@Toffikk
Toffikk changed the base branch from main to dev/vanilla-fix-comments July 15, 2026 17:51
@Toffikk
Toffikk force-pushed the dev/require-docs-for-configs branch 2 times, most recently from 5dd663e to 6033499 Compare July 15, 2026 17:59
@Toffikk
Toffikk force-pushed the dev/require-docs-for-configs branch from 6033499 to bed15a4 Compare July 15, 2026 18:06
@Dueris

Dueris commented Jul 15, 2026

Copy link
Copy Markdown
Member

I'm not sure about this. The configs that don't have a config are sorta supposed to be the ones that are self explanatory as to avoid cluttering the config with a ton of documentation(bc then it looks too busy)

@Toffikk

Toffikk commented Jul 15, 2026

Copy link
Copy Markdown
Member Author

I'm not sure about this. The configs that don't have a config are sorta supposed to be the ones that are self explanatory as to avoid cluttering the config with a ton of documentation(bc then it looks too busy)

Then they can just be annotated as Undocumented like in this pr, perhaps with a more fitting reason or a simple doesn’t require docs

@Toffikk

Toffikk commented Jul 15, 2026

Copy link
Copy Markdown
Member Author

This is more so as a safeguard to make sure config options that don’t have comments are intended not to have them. With this change i discovered some like purpur alt keepalive not being documented

@Toffikk
Toffikk force-pushed the dev/require-docs-for-configs branch from fcacd66 to 7d42f8c Compare July 15, 2026 20:18
Base automatically changed from dev/vanilla-fix-comments to main July 15, 2026 20:34
@Toffikk
Toffikk requested review from a team, Dueris, MrMasrozYTLIVE and R00tB33rMan and removed request for a team and R00tB33rMan July 28, 2026 20:36
@MrMasrozYTLIVE
MrMasrozYTLIVE requested review from a team, Dueris and MrMasrozYTLIVE and removed request for a team, Dueris and MrMasrozYTLIVE July 28, 2026 20:39
@Toffikk
Toffikk requested review from a team and R00tB33rMan and removed request for a team, Dueris, MrMasrozYTLIVE and R00tB33rMan July 28, 2026 20:39

@Dueris Dueris left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

After these are addressed and Bacons other PR that is currently blocking this is merged, Im good with this being merged too

"each bucket will be ticked"
).greaterThan(0.0F);
option("purpurAlternativeKeepalive")
.docs(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This method(docs) wraps the text in the blob. Probably best to use a Style instance and make the AKA specifically on a new line rather than wrapped with everything else.

public boolean disableSweepingEdge = false;
@Undocumented("Doesn't require docs.")
public boolean disableCritsWhileSprinting = false;
// public int invulnerabilityTicks = 10;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Perhaps out of scope of this PR but we can probably remove this comment too since there isn't a reliable way to reintroduce this given our new setup

}

if (Part.class.isAssignableFrom(field.getType())) {
@SuppressWarnings("unchecked")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Use a comment to suppress the warnings to follow the rest of the code-style:

//noinspection unchecked

final Set<String> returned = new HashSet<>();
final Set<String> fixed = new HashSet<>();

for (final var element : found) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

While I honestly dont care TOO much, use the actual type instead of var lol

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request priority: low status: blocked Rejected or should not be merged at this time for a certain reason(or multiple if applicable) status: waiting for review Waiting for review from an official member of the team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants