RFC 0070: Treat dashes and underscores as the same when comparing mod ids#70
Open
AlexIIL wants to merge 4 commits intoQuiltMC:mainfrom
Open
RFC 0070: Treat dashes and underscores as the same when comparing mod ids#70AlexIIL wants to merge 4 commits intoQuiltMC:mainfrom
AlexIIL wants to merge 4 commits intoQuiltMC:mainfrom
Conversation
Leo40Git
suggested changes
May 6, 2023
|
|
||
| ## Summary | ||
|
|
||
| Treat dashes and underscores as the same for mod id comparison purposes. |
Contributor
There was a problem hiding this comment.
Suggested change
| Treat dashes and underscores as the same for mod id comparison purposes. | |
| Treat dashes and underscores as the same for mod identifier (ID) comparison purposes. |
|
|
||
| ## Motivation | ||
|
|
||
| An occasional cause for confusion is when two different mods only differ by underscores (`_`) and dashes(`-`) in their mod-ids. Since those characters aren't usually used to discriminate between mods, I propose we consider them as the same for the purposes of mod resolution and solving. |
Contributor
There was a problem hiding this comment.
Suggested change
| An occasional cause for confusion is when two different mods only differ by underscores (`_`) and dashes(`-`) in their mod-ids. Since those characters aren't usually used to discriminate between mods, I propose we consider them as the same for the purposes of mod resolution and solving. | |
| An occasional cause for confusion is when two different mods only differ by underscores (`_`) and dashes(`-`) in their mod IDs. Since those characters aren't usually used to discriminate between mods, I propose we consider them as the same for the purposes of mod resolution and solving. |
|
|
||
| ## Explanation | ||
|
|
||
| I propose we normalise all quilt mod-ids by replacing every dash (`-`) and underscore (`_`) with an internal character (likely `#`, but any character that's not permitted by the specification, and is unlikely to be part of the specification in the future, would work. Ascii is preferable due to string optimisations though). |
Contributor
There was a problem hiding this comment.
Suggested change
| I propose we normalise all quilt mod-ids by replacing every dash (`-`) and underscore (`_`) with an internal character (likely `#`, but any character that's not permitted by the specification, and is unlikely to be part of the specification in the future, would work. Ascii is preferable due to string optimisations though). | |
| I propose we normalise all Quilt mod IDs by replacing every dash (`-`) and underscore (`_`) with some internal character (one that is not permitted by the specification). |
Moved this discussion down to the "Unresolved questions" section.
|
|
||
| This normalisation would purely happen internally to quilt-loader, and wouldn't be shown to users, or exposed in methods like `ModContainer.getId()`. Instead this would allow dependencies and breaks to accidently target the wrong ID without causing issues, and would automatically cause mods with underscores or dashes to "provide" the other representations automatically. | ||
|
|
||
| For example, a mod with an ID "potion_craft-expanded" would result in: |
Contributor
There was a problem hiding this comment.
Suggested change
| For example, a mod with an ID "potion_craft-expanded" would result in: | |
| For example, with a mod with the ID "potion_craft-expanded": |
| For example, a mod with an ID "potion_craft-expanded" would result in: | ||
| - `QuiltLoader.getModContainer("potion_craft-expanded")` would return the mod | ||
| - `QuiltLoader.getModContainer("potion-craft-expanded")` would return the same mod | ||
| - The same mod is also returned for strings `potion_craft_expanded` and `potion-craft_expanded` |
Contributor
There was a problem hiding this comment.
Suggested change
| - The same mod is also returned for strings `potion_craft_expanded` and `potion-craft_expanded` | |
| - The same mod is also returned for `potion_craft_expanded` and `potion-craft_expanded` |
| - `QuiltLoader.isModLoaded` would return true for all 4 strings | ||
| - `QuiltLoader.getEntrypoints` would return the same entrypoint list for each id. | ||
| - `QuiltLoader.getAllMods()` will only contain a single entry | ||
| - `ModMetadata.provides()` would NOT contain provided entries for every possible combination, instead it would be empty (if no provided entries exist in the quilt.mod.json) |
Contributor
There was a problem hiding this comment.
Suggested change
| - `ModMetadata.provides()` would NOT contain provided entries for every possible combination, instead it would be empty (if no provided entries exist in the quilt.mod.json) | |
| - `ModMetadata.provides()` would NOT contain provided entries for every possible combination, instead it would be empty (unless the mod defines `provided` entries in its quilt.mod.json) |
| ## Rationale and Alternatives | ||
|
|
||
| We could instead normalise all dashes to underscores (or vice versa) but that has a few issues: | ||
| * During debugging, it won't be easy to tell if a string contains a normalised modid or a non-normalised id, leading to more bugs. |
Contributor
There was a problem hiding this comment.
Suggested change
| * During debugging, it won't be easy to tell if a string contains a normalised modid or a non-normalised id, leading to more bugs. | |
| * During debugging, it won't be easy to tell if a string contains a normalised mod ID or a non-normalised id, leading to more bugs. |
| * We'd have to choose a favourite character, which could result in a fairly lengthy debate. Sidestepping that seems useful. | ||
|
|
||
| We could even remove both characters entirely during normalisation, so `quilt_loader` would get normalised to `quiltloader`. However this has another problem, in addition to those listed above: | ||
| * It disallows the use of separator characters in mod-ids, which are genuinly quite useful. |
Contributor
There was a problem hiding this comment.
Suggested change
| * It disallows the use of separator characters in mod-ids, which are genuinly quite useful. | |
| * It disallows the use of separator characters in mod IDs, which are genuinely quite useful. |
|
|
||
| ## Prior Art | ||
|
|
||
| I'm not aware of forge, fabric, or quilt related projects doing this before. |
Contributor
There was a problem hiding this comment.
Suggested change
| I'm not aware of forge, fabric, or quilt related projects doing this before. | |
| I'm not aware of Forge, Fabric, or Quilt related projects doing this before. |
Comment on lines
+48
to
+49
| I'm not certain what character should be used, but '#' seems like a good candidate. | ||
| I'm not sure if calls to `QuiltLoader.getModContainer` or `QuiltLoader.isModLoaded` using the normalised ID should return the mod, null, or if that should be left unspecified. |
Contributor
There was a problem hiding this comment.
Suggested change
| I'm not certain what character should be used, but '#' seems like a good candidate. | |
| I'm not sure if calls to `QuiltLoader.getModContainer` or `QuiltLoader.isModLoaded` using the normalised ID should return the mod, null, or if that should be left unspecified. | |
| - I'm not certain what character should be used, but '#' seems like a good candidate. Any character that's not permitted by the specification, and is unlikely to be part of the specification in the future, would work - though, ASCII is preferable due to string optimisations. | |
| - I'm not sure if calls to `QuiltLoader.getModContainer` or `QuiltLoader.isModLoaded` using the normalised ID should return the mod, null, or if that should be left unspecified. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Rendered View