UserFeatures config problems
kristofbolyai opened this issue ยท 6 comments
I was going to create a PR fixing this, but this one is a controversial topic, so I thought an issue might be better to start with.
ConfigManager
has a requirement that a Feature
must be annotated with FeatureInfo
to get the Feature
's category or else config options won't load. This is obviously not a good thing.
Here is the things we could do:
- Make
FeatureInfo
annotation required by throwing Exception on feature registration - Make
ConfigManager
assume blank ("") category for features without annotation (the annotation already has""
as default category).
@P0keDev @magicus @Incompleteusern What do you think?
maybe i'm missing something, but i don't really see what the issue is - like you said, the annotation already has a default category defined, so config options will always be loaded as long as it's present (which is required for other feature loading code anyway iirc)
to deal with translations and hierarchy definitions, the category field will probably become an enum eventually, so the default could just be UNCATEGORIZED or something similar
maybe i'm missing something, but i don't really see what the issue is - like you said, the annotation already has a default category defined, so config options will always be loaded as long as it's present (which is required for other feature loading code anyway iirc)
to deal with translations and hierarchy definitions, the category field will probably become an enum eventually, so the default could just be UNCATEGORIZED or something similar
No, actually the annotation is not required as of right now and not every feature has it. So for example, many UserFeatures don't load the userEnabled config (and you can't change it hence).
No, actually the annotation is not required as of right now and not every feature has it. So for example, many UserFeatures don't load the userEnabled config (and you can't change it hence).
ah, i didn't realize that.
the userEnabled switch should be categorized even if there's no other options, so i think requiring FeatureInfo for loading configs is fine. i'm not sure if it should be enforced though, because it would be unnecessary for a lot of InternalFeatures - maybe only enforce it for UserFeature
the userEnabled switch should be categorized even if there's no other options, so i think requiring FeatureInfo for loading configs is fine. i'm not sure if it should be enforced though, because it would be unnecessary for a lot of InternalFeatures - maybe only enforce it for UserFeature
Enforcing it if a feature has configs should be the way to go imo.
Do youreally need to ask what I think? I thought I've made my views on FeatureInfo clear by now. ๐
To iterate: I don't think FeatureInfo provides much value. I don't wish to see it required. I'd rather see it removed. :-) I have still hard to see really how the category will help us. Maybe it will be more obvious once we have a config GUI. Or a config GUI will show us that we need a different kind of grouping than what the category provides, so we'll have to redo it anyway.
So my stance here is that if a FeatureInfo is missing, we should still be able to use @Config
, and you can assume the empty category (or whatever default it might be replaced with in the future).