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

added the option to specify categories and subcategories with the @Category and @SubCategory annotations. #292

Closed
wants to merge 16 commits into from

Conversation

urisinger
Copy link

@urisinger urisinger commented Oct 25, 2023

Description

This allows mod developers to better organize their code by creating a diffrent class for each category/subcategory of options they have, this also allows the devolper to create custom categories. This will not effect the current api, which can still be used.

Documentation

This feature adds 2 new annotions, @category and @SubCategory, Which both need to be documanted.

Checklist

  • I made a clear description of what was changed
  • I stated why these changes were necessary
  • I updated documentation or said what needs to be updated
  • I made sure these changes are backwards compatible
  • This pull request is for one feature/bug fix

Copy link
Member

@pauliesnug pauliesnug left a comment

Choose a reason for hiding this comment

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

it looks good, just not 100% sure of the rationale behind this change and a couple formatting/api status nitpicks. also run generate abi

optionNames.put(optionName, option);
}
}
logger.trace("Finished generating option list for {} subcategory (targetting={})", mod.name, targetClass.getName());
Copy link
Member

Choose a reason for hiding this comment

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

a lot of this code is reused from addCategory, maybe they could be combined

Copy link
Author

Choose a reason for hiding this comment

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

they look simular but they pass a diffrent amount of arguments to some methods, so it wouldnt really work. i could try tho

Copy link
Member

Choose a reason for hiding this comment

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

i presume its not possible to abstractify this? they really do look very similar

@urisinger
Copy link
Author

urisinger commented Oct 25, 2023

it looks good, just not 100% sure of the rationale behind this change and a couple formatting/api status nitpicks.

In my opinion this way of specifying categories is much better, and leads to less confusing varible names. Also wyvest said it would be better, so how could it not?

@urisinger urisinger requested a review from pauliesnug October 26, 2023 19:12
@Wyvest
Copy link
Member

Wyvest commented Nov 21, 2023

Sorry for the delayed responses from Polyfrost - we've all been going through IRL stuff like school and other related activities

can you run the apiDump gradle command? That should generate the new ABI with changes made in this PR.

Wyvest
Wyvest previously approved these changes Nov 22, 2023
Copy link
Member

@Wyvest Wyvest left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@pauliesnug pauliesnug left a comment

Choose a reason for hiding this comment

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

lgtm

@@ -215,7 +219,7 @@ protected final void generateOptionList(Object instance, OptionPage page, Mod mo
}

/**
* Generate the option list, for internal use only
* Generate the option list, for use only
Copy link
Member

Choose a reason for hiding this comment

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

for [...] use only?

optionNames.put(optionName, option);
}
}
logger.trace("Finished generating option list for {} subcategory (targetting={})", mod.name, targetClass.getName());
Copy link
Member

Choose a reason for hiding this comment

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

i presume its not possible to abstractify this? they really do look very similar

@Target(ElementType.FIELD)
public @interface SubCategory {
String name();

Copy link
Member

Choose a reason for hiding this comment

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

why is there a random return here -- also these two categories dont have any autocompletion or recommended category names? we dont want 100 categories one from each mod -- it will be a pain to look at, manage, and do i18n for.

Copy link
Member

Choose a reason for hiding this comment

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

(return means a line space btw, you might not have been confused but i was very confused so)

Copy link
Member

@nextdayy nextdayy left a comment

Choose a reason for hiding this comment

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

  • please abstract out the method paulie mentioned
  • please run "reformat code" to align it with our code quality standards

@urisinger
Copy link
Author

urisinger commented Nov 25, 2023

I have a problem with running mc in the devolpment environment, so i cant test these changes right now, but this should work and reduce some code duplication

@urisinger
Copy link
Author

Stopped working on this since 1.0 is releasing, I hope that 1.0 won't also have this problem

@urisinger urisinger closed this Jan 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants