Extend plugin attributes with optional fields #20
Conversation
|
Download the artifacts for this pull request: |
hazre
requested review from
EIA485,
LeCloutPanda,
NepuShiro,
art0007i and
knackrack615
|
| public BepInPlugin(string GUID, string Name, string Version) | ||
| /// <param name="Author">The author of the plugin.</param> | ||
| /// <param name="Link">The link to the plugin's website or repository.</param> | ||
| public BepInPlugin(string GUID, string Name, string Version, string Author = null, string Link = null) |
There was a problem hiding this comment.
breaks binary compatibility, also makes no sense to do this since we already have a lot of mods which use ResonitePlugin from the shim, and you can't just delete the attribute from the shim without breaking all already released mods.
There was a problem hiding this comment.
theoretically we if all current plugins reference the shim's ResonitePlugin, we would just need to push an update to that and they would work
| public PatcherPluginInfoAttribute(string GUID, string Name, string Version) | ||
| /// <param name="Author">The author of the plugin.</param> | ||
| /// <param name="Link">The link to the plugin's website or repository.</param> | ||
| public PatcherPluginInfoAttribute(string GUID, string Name, string Version, string Author = null, string Link = null) |
|
In general I'm against merging this at all. I don't think we should be putting resonite specific stuff in bepinex codebase |
|
Okay so you guys are fine with doing two complete different things for plugin vs prepatchers? [ResonitePlugin(PluginMetadata.GUID, PluginMetadata.NAME, PluginMetadata.VERSION, PluginMetadata.AUTHORS, PluginMetadata.REPOSITORY_URL)]
public class Plugin : BepInPlugin[PatcherPluginInfo(PluginMetadata.GUID, PluginMetadata.NAME, PluginMetadata.VERSION)]
public class Patcher : BasePatcherThis is the inconsistency that I would like to fix while we can. There is only one or two mods that use |
|
yes since we have no use for the metadata from prepatchers |
|
If this were to implemented it would only be implemented for the sake of consistency, there is no actual functionality gain by having metadata in prepatchers, they are not visible anywhere. |
|
Just because it's not implemented right now, doesn't mean we wouldn't use it in the future. Prepatcher only mods are still valid use case. Like how yesterday I was trying to make a prepatcher mod that removes RML's mod settings. |
|
We can only do this kind of breaking change right now, if we tomorrow release bepis, it would be impossible to do. That's why its a good idea to make them consistent and future proof it. |
|
currently our only usecase for this metadata is showing it in the mod settings. prepatchers cant have configuration options. |
I guess the argument here is if we ever decide in the future to list prepatchers or do something with them in any kind of UI, it'd be impossible to do. |
|
So after discussing this in Discord with everyone, we came to the conclusion that this is not needed as of right now as it is very unlikely to utilize the metadata, and this way we're not modifying original BepInEx behavior. |
This adds
AuthorandLinkas optional attribute fields, meaning it's still valid if only the main 3 bepiex expects are present.This should make prepatchers more consistent with normal plugins.