[core] Deprecate string property and string multi property#1501
Conversation
Most of the time they can't be changed right now
Generated by 🚫 Danger |
adangel
left a comment
There was a problem hiding this comment.
Can you have a look, whether we need the delim(",") call for the stringListProperties?
We should probably make the properties of Rule (violationSuppression**) and XPathRule (xpath, version) and all properties of all renderers private/package private in 7.0.0. I don't see a use case to have them public. Wdyt?
| * matching a regular expression. | ||
| */ | ||
| // TODO 7.0.0 use PropertyDescriptor<Pattern> | ||
| StringProperty VIOLATION_SUPPRESS_REGEX_DESCRIPTOR = new StringProperty("violationSuppressRegex", |
There was a problem hiding this comment.
Ok, we can't switch it now, because the properties are a field of the interface Rule - which makes it public API.
Maybe we should remove it here in the (Java) API in 7.0.0? The only API left would be, when using a rule and setting the properties in the ruleset xml. There the type doesn't matter - since the String is then automatically converted into a Pattern.
There was a problem hiding this comment.
Yeah it would make sense to hide the descriptors. But we should probably allow setting these from Java somehow, for external tools that edit rules. The descriptors are definitely an implementation detail
| public class XPathRule extends AbstractRule { | ||
|
|
||
| // TODO 7.0.0 use PropertyDescriptor<String> | ||
| public static final StringProperty XPATH_DESCRIPTOR = StringProperty.named("xpath") |
There was a problem hiding this comment.
Same here - the properties of XPathRule shouldn't probably be public.
Maybe tag them for now as @internalapi?
There was a problem hiding this comment.
In fact I think for 7.0.0 XPathRule as a whole should be internal API. Since we're introducing XML sugar for XPath rules, we could jump on the occasion to completely hide away the implementation of XPath rules, and only provide a set of interfaces as API. That would allow language implementations to use different XPath rule classes at their discretion, and us to change the implementation transparently for any users. XPathRule's API would anyway be significantly broken by dropping Jaxen...
There was a problem hiding this comment.
Yes, this makes sense, having XPathRule as whole internal API.
| public class YAHTMLRenderer extends AbstractAccumulatingRenderer { | ||
|
|
||
| public static final String NAME = "yahtml"; | ||
| // TODO 7.0.0 use PropertyDescriptor<Optional<File>> with a constraint that the file is an existing directory |
| */ | ||
| public class GenericClassCounterRule extends AbstractJavaRule { | ||
|
|
||
| // Class is unused, properties won't be converted |
There was a problem hiding this comment.
On first look, this rule might be use case for multi file analysis...
There was a problem hiding this comment.
Maybe... We should probably scrap the class and add its purpose somewhere on the roadmap
There was a problem hiding this comment.
Yes, no need to keep this class - we should just keep the idea/purpose of the rule.
Refs #1432
Many usages cannot be replaced just yet, because they are published API. Interestingly, AvoidDuplicateLiterals tried to implement its own StringMultiProperty based on a StringProperty, which makes it really hard to refactor. I think it's better to leave it like that until 7.0.0.
There's surprisingly nearly nothing to do for #1027, and all of it will have to wait for 7.0.0. I suppose we don't use so many regexes.