Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
The Mod Protocol API, aka better validation of mod presence of both-side mods with extra protocol version syncing #4011
base: 1.21.1
Are you sure you want to change the base?
The Mod Protocol API, aka better validation of mod presence of both-side mods with extra protocol version syncing #4011
Changes from 6 commits
605d858
5d1f970
dbcdcaa
685051a
d16dbc4
781a8da
822bdb2
73affdf
a770fb7
175d392
a08bffa
ae341a5
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use
@Override
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question (ive not looked at the impl), where does this ID come from when a protocol is specified in the fabric.mod.json?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By default it's "mod:(modid)", but can be overriten with any other identifier
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Somewhat realted to my above question, why are these needed, would it be better tieing this to the
ModContainer
/modid?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since (currently) mod can provide multiple protocols (or make them in code) it still makes sense for this to exist. As a bonus people might want to have it different/more specific with something like
feature
namespace.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do think tieing it to the mod solves a lot of problems mostly by removing a lot of duplicated data from the fmj. You also get support for "provides" for free.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Explain what these do and are used for in the javadoc. I have no clue just looking at the API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By default mostly to indicate the source and display priority, special being always displayed first, then mod and ending up with feature. But these can be added upon in event-ordering styled fashion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd just keep it simple and sort alphabetically for now. Maybe add it later as a follow up, or even have a custom UI drawn by the client making it easier to consume the data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorting is already done and yeah it still sorts alphabetically for paths. It's mostly useful as another mod could register a modpack version/name in the special namespace, making it always display as the first thing