[core] Implement PropertyFactory by oowekyala · Pull Request #1470 · pmd/pmd

oowekyala · 2018-11-16T11:56:25Z

Second step to #1432, scheduled for 6.10.0. It should be merged on master. The release notes update about PropertyFactory will be part of the next PR, along with the deprecations of concrete property classes.

The API is built around PropertyFactory. The new concrete classes of the property descriptors are not public. The PropertyBuilder replaces the previous hierarchy in n.s.pmd.properties.builders
The PropertyFactory and property builders are put in place to provide a replacement to the current public property classes that will be nearly 100% source compatible with the 7.0.0 version (with the exception of the delimiter() method).
The new API is designed to offer exactly the same capabilities to rule builders as the current framework for now, but no more. Functionality will be fleshed out on the 7.0.x branch, because without java 8 it's a pain. In particular the PropertyConstraint interface and its usage can be much improved with java 8.
If a rule has a property with a wrong value, it is now reported as a configuration error and the rule is pruned from the run ruleset. This contrasts with the current framework, which throws an IllegalArgumentException if a constraint e.g. on numeric range is violated.

ghost · 2018-11-16T17:11:55Z

	1 Message
📖	This changeset introduces 0 new violations and 0 new errors, removes 0 violations and 0 errors. Full report
✅	This changeset introduces 0 new violations and 0 new errors, removes 61 violations and 0 errors. Full report

Generated by 🚫 Danger

They make no sense as they are shared by all StatisticalRules, and provoke configuration errors on normal inputs since uncorrectly configured properties now cause the rule not to be run

adangel

I like the way, this is going 👍

One small question: We once said, we wanted to move the internal implementation classes into a sub-package called internal or impl. You have now created the implementations in the same package and just made the classes package-private - which is fine, since this also hides the classes from public API. I think, if we would create a sub-package, then you'd need to make the implementations public, so that you can use it the PropertyFactory... What do you think?

adangel · 2018-11-23T18:22:45Z

    // todo Java 8 move up to interface
    @Override
    public String dysfunctionReason() {
+        for (PropertyDescriptor<?> descriptor : getOverriddenPropertyDescriptors()) {


We should think about improving this API (different PR) - this would only return the first error. We should have a API, that returns all the problems a rule has.

adangel · 2018-11-23T18:30:20Z

+ * Note: from 7.0.0 on, this will be the only way to
+ * build property descriptors.
+ *
+ * TODO next PR add doc


adangel · 2018-11-23T19:00:28Z

+     *
+     * @return The same builder
+     */
+    // TODO we could probably specify the order of execution of constraints come 7.0.0, for now this remains unspecified


I thought shortly about this - but does the order of the constraints matter? They all need to be met, and the constraints should be independent from each other...

Indeed it wouldn't matter if constraints are truly independent -> in which case we should specify that. I think it makes sense, and it would allow to report all errors on a value, like you suggested for rules.

Co-Authored-By: oowekyala <clement.fournier76@gmail.com>

oowekyala · 2018-11-23T20:03:51Z

One small question: We once said, we wanted to move the internal implementation classes into a sub-package called internal or impl. You have now created the implementations in the same package and just made the classes package-private - which is fine, since this also hides the classes from public API. I think, if we would create a sub-package, then you'd need to make the implementations public, so that you can use it the PropertyFactory... What do you think?

My 2 cents on that is, if the internal classes are only relevant to the package they're in (eg in this case), then I'd prefer package visibility over having an internal package. The internals are better hidden that way I guess. Although in some cases using package visibility is simply not possible, then those internal APIs would belong in an internal package. Also if the main package has many classes.

…nto new-properties-framework

jsotuyod · 2019-01-16T03:37:31Z

+                public List<T> get() {
+                    return new ArrayList<>();
+                }
+            };


do we really need this and to pass it around just to create an empty list? sounds like overkill to me

This is in prevision of the migration to Java 8. Many things here are sloppy because I designed this as if it was Java 8, but had to write it with Java 7 syntax so it's ugly. In Java 8 it will be as short as ArrayList::new, and we can also provide a toSet method with HashSet::new, or let the user use any collection supplier they want.

It's not about the syntax, it's about the actual need for a Supplier for such a thing

As documented, GenericCollectionPropertyBuilder is designed to support building properties for an arbitrary Collection, not just lists. The most generic way to do that is to pass on a Supplier for the collection you want. How would you do it otherwise?

I'm unsure there is a need for arbitrary collections types at all, and I fear being flexible about it is overengineering. Do we even have a use case for this?

It's extremely easy to grab, so I wouldn't consider that overengineering. Everything is already completely generic, and GenericMultivaluePropertyDescriptor will be removed with 7.0.0, so there's really no point in artificially constraining ourselves to lists. One use case I can think of is the options for metrics rules, which would be better served by using a Set rather than a list.

oowekyala added 15 commits November 5, 2018 00:33

WIP

972a991

Move towards java7 compat

b9a15b4

Try to integrate in current framework

de17638

Fix compilation

993d80d

WIP

c4bf373

Simplify builder hierarchy

356dfbf

Generalize list property builders

a08c370

Checkstyle

21f0150

Merge branch 'master' into new-properties-framework

877ed5e

Merge branch 'master' into new-properties-framework

e7dc953

Cleanup

c4deebb

Remove the uiOrder from the new builder

c3af76b

Add configuration error if property has wrong value

f3e618b

Test

d0401eb

Add some more tests

0fd8f91

oowekyala added the is:waiting-for-review label Nov 16, 2018

oowekyala added this to the 6.10.0 milestone Nov 16, 2018

oowekyala mentioned this pull request Nov 16, 2018

[core] Implementation plan for property framework changes #1432

Closed

36 tasks

oowekyala added 2 commits November 16, 2018 13:05

checkstyle

d662094

Fix build

2229905

Remove constraints on StatisticalRule descriptors

f0dbb36

They make no sense as they are shared by all StatisticalRules, and provoke configuration errors on normal inputs since uncorrectly configured properties now cause the rule not to be run

oowekyala mentioned this pull request Nov 16, 2018

Support configuration errors in the report pmd/pmd-regression-tester#57

Closed

adangel reviewed Nov 23, 2018

View reviewed changes

adangel removed the is:waiting-for-review label Nov 23, 2018

adangel and others added 2 commits November 23, 2018 20:25

Fix wrong issue link

76e57ea

Co-Authored-By: oowekyala <clement.fournier76@gmail.com>

Fix outdated versions

ce83a87

Co-Authored-By: oowekyala <clement.fournier76@gmail.com>

oowekyala added 2 commits November 25, 2018 22:34

Use Comparable for generic constraint

1344b99

Remove ui order

bf8cb13

Merge branch 'new-properties-framework' of github.com:oowekyala/pmd i…

b2d9b4e

…nto new-properties-framework

adangel approved these changes Nov 26, 2018

View reviewed changes

adangel self-assigned this Nov 26, 2018

adangel merged commit b2d9b4e into pmd:master Nov 26, 2018

oowekyala deleted the new-properties-framework branch November 26, 2018 21:00

jsotuyod reviewed Jan 16, 2019

View reviewed changes

Comment thread pmd-core/src/main/java/net/sourceforge/pmd/properties/GenericMultiValuePropertyDescriptor.java

jsotuyod reviewed Jan 16, 2019

View reviewed changes

Uh oh!

Conversation

oowekyala commented Nov 16, 2018

Uh oh!

ghost commented Nov 16, 2018 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adangel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

oowekyala commented Nov 23, 2018

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ghost commented Nov 16, 2018 •

edited by ghost

Loading