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

Begin QMJ Writer #446

Closed
wants to merge 4 commits into from
Closed

Begin QMJ Writer #446

wants to merge 4 commits into from

Conversation

OroArmor
Copy link
Member

@OroArmor OroArmor commented Aug 8, 2024

This will allow people to make tools to convert fabric projects into quilt projects without needing to re-implement the whole QMJ spec

@OroArmor OroArmor added the enhancement New feature or request label Aug 8, 2024
@OroArmor OroArmor marked this pull request as ready for review August 10, 2024 06:20
Copy link
Contributor

@AlexIIL AlexIIL left a comment

Choose a reason for hiding this comment

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

I'm somewhat conflicted about this - I agree that this functionality is useful (and needed), but unhappy about adding apis to internal packages. I can understand that there isn't really a better place for this though - it's only intended to be used by external tools that will already have to use internal code like org.quiltmc.loader.impl.metadata.FabricModMetadataReader and some related classes.

Overall the code is reasonable, with a couple of major problems:

  • Reflection shouldn't be used to access specific fields or classes inside the same project - especially if access can be made package-private. Alternately the getIcon method could be made optional methods in InternalModMetadata, Icons, and FabricLoaderModMetadata.
  • Version ranges as arrays are deprecated by the QMJ spec, in preference of explicit all/any specifiers. I've included some code which hopefully fixes this?

@OroArmor
Copy link
Member Author

I'm somewhat conflicted about this - I agree that this functionality is useful (and needed), but unhappy about adding apis to internal packages. I can understand that there isn't really a better place for this though - it's only intended to be used by external tools that will already have to use internal code like org.quiltmc.loader.impl.metadata.FabricModMetadataReader and some related classes.

Yeah that's the problem. It both needs to be "API" but also not. It probably wont get run when loader is actually running, its just something that a tool can use when compiled against loader.

@OroArmor OroArmor closed this Aug 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants