[core] Improvements and planned updates around PropertySource#1322
Conversation
Generated by 🚫 Danger |
adangel
left a comment
There was a problem hiding this comment.
This is almost ready, sorry for the late review!
I only see these points, before we can merge:
- reintroduce
hasOverriddenProperty(...)as deprecated method just delegating to the newisPropertyOverridden(...)in RuleReference. - adding the deprecated classes/methods in the release notes under "API Changes"
- making
getPropertySourceType(...)protected in AbstractPropertySource
The other comments are just thoughts. We probably need to define more in detail, what one can override exactly when defining a rule reference. Like you mentioned, overriding the language doesn't make sense. The examples handling can be simplified (I see a use case for overriding the examples). We also probably shouldn't allow to define new properties on a rule reference...
| } | ||
|
|
||
| // FIXME should we really allow overriding the language of a rule? | ||
| // I don't see any case where this wouldn't just make the rule fail during execution |
There was a problem hiding this comment.
We probably should throw something like "UnsupportedOperationException" in 7.0.0.
Most likely, you can't reuse a rule for other languages - I think, right now, all our rules are tight to a specific AST.
It could work, if the rule only uses Node and no subclass. For XPath-rules, I don't see, how it can work (unless the AST node names, are the same for two languages...).
So, better we stop here with a exception and get notified that way, when such a real use case pops up.
There was a problem hiding this comment.
We could argue similar with setMinimumLanguageVersion / setMaximumLanguageVersion. You could override these properties and effectively disable a rule, but why would someone want to do this, if one can simply remove the rule reference from the ruleset?
|
|
||
| @Override | ||
| public void addExample(String example) { | ||
| // TODO Intuitively, if some examples are overridden (even with empty value), then |
There was a problem hiding this comment.
I think, having the possibility to override the example could be used for generating documentation (or displaying documentation in IDE plugins): E.g. someone creates a custom ruleset and wants to provide translated/adapted examples that better fit the project at hand.
I agree, that we should keep the overriding mechanism simple: If a rule reference is provided with examples, then we should remove all original examples and just use the ones provided by the rule reference.
| @@ -246,14 +277,16 @@ public void definePropertyDescriptor(PropertyDescriptor<?> propertyDescriptor) t | |||
| propertyDescriptors.add(propertyDescriptor); | |||
There was a problem hiding this comment.
Does it make sense, to allow defining additional properties on a rule reference? The original rule can't make any sense of a new property...
| public <T> void setProperty(PropertyDescriptor<T> propertyDescriptor, T value) { | ||
| // Only override if different than current value. | ||
| if (!isSame(super.getProperty(propertyDescriptor), value)) { | ||
| if (!Objects.equals(super.getProperty(propertyDescriptor), value)) { |
There was a problem hiding this comment.
I think, this comparison won't work with RegexProperties... (it didn't work with isSame, too).
Actually, it does not hurt so much here - but it will always override the property, even if the pattern is the same.
Any two java.util.regex.Pattern instances are considered not equal.
| || super.hasDescriptor(descriptor); | ||
| } | ||
|
|
||
| public boolean hasOverriddenProperty(PropertyDescriptor<?> descriptor) { |
There was a problem hiding this comment.
For compatibility reasons, we'll need to keep hasOverriddenProperty(...) as a deprecated method. It's public...
| * | ||
| * @return the name | ||
| */ | ||
| public abstract String getName(); |
There was a problem hiding this comment.
For compatibility reasons, we need to keep getName(), don't we?
There was a problem hiding this comment.
Ah, I see, it's in the PropertySource interface now. Good :)
| * @return the name | ||
| */ | ||
| public abstract String getName(); | ||
| private String getPropertySourceType() { |
There was a problem hiding this comment.
We probably should make this protected and abstract, so that the AbstractPropertySource doesn't need to know its concrete subclasses... like Rule.
|
Merged! FYI - I've fixed the points I mentioned. |
| @Override | ||
| public Map<PropertyDescriptor<?>, Object> getOverriddenPropertiesByPropertyDescriptor() { | ||
| return propertyValues; | ||
| return propertyValues == null ? new HashMap<PropertyDescriptor<?>, Object>() : new HashMap<>(propertyValues); |
There was a problem hiding this comment.
why not Collections.emptyMap()?
I was working on making required properties a thing for #973 (the last comments), and it occurred to me that for that to work, PropertySources should provide kind of the same functionality as RuleReferences: track the overridden properties. This was already kind of the case, except it wasn't documented.
This PR brushes up on PropertySource, fixing some inconsistencies, deprecating unneeded API, and making it consistent with RuleReference. The goal for 7.0.0 would be to have RuleReference extend AbstractPropertySource to share the implementation. This can't be done right now as making AbstractDelegateRule extend AbstractPropertySource would break the contract of AbstractDelegateRule by baking in some property overriding facilities. Since AbstractDelegateRule is only used by RuleReference anyway, I think we can remove it without regret come 7.0.0