vaadin / flow

Vaadin Flow is a Java framework binding Vaadin web components to Java. This is part of Vaadin 10+.
Apache License 2.0
618 stars 167 forks source link

Protected constructor Binder(PropertySet) should have Javadoc link to static method Binder.withPropertySet() #13145

Open archiecobbs opened 2 years ago

archiecobbs commented 2 years ago

Describe your motivation

I have a use case where I want to create a Binder using a PropertySet, but I can't do that directly because the constructor Binder(PropertySet) is protected.

There's no reason that I can see for this constructor to be protected; instead, it should be public.

The use case here is a programmable form that is definable in XML and has named parameters of various types. In other words, the parameter names and types just happen to come from another source than a Java bean. But there's nothing wrong with that.

Describe the solution you'd like

diff --git a/flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java b/flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java
index 4d6ee89a6f..8ad5cd265d 100644
--- a/flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java
+++ b/flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java
@@ -1667,7 +1667,7 @@ public class Binder<BEAN> implements Serializable {
      * @param propertySet
      *            the property set implementation to use, not <code>null</code>.
      */
-    protected Binder(PropertySet<BEAN> propertySet) {
+    public Binder(PropertySet<BEAN> propertySet) {
         Objects.requireNonNull(propertySet, "propertySet cannot be null");
         this.propertySet = propertySet;
     }

Describe alternatives you've considered

Creating a custom subclass that does nothing except have the same constructor but made public. Obviously this works but is just a workaround.

Additional context

This appears to be a simple design flaw. I see no reason why this constructor shouldn't be public. But maybe there is one - if so, please enlighten me.

archiecobbs commented 2 years ago

Argh, I just realized that there is a static method for doing this: Binder.withPropertySet().

OK so ignore previous patch. Maybe the thing to do is make this a little more obvious with this patch?

diff --git a/flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java b/flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java
index 4d6ee89a6f..3d651d0dab 100644
--- a/flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java
+++ b/flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java
@@ -1666,6 +1666,7 @@ public class Binder<BEAN> implements Serializable {
      *
      * @param propertySet
      *            the property set implementation to use, not <code>null</code>.
+     * @see #withPropertySet
      */
     protected Binder(PropertySet<BEAN> propertySet) {
         Objects.requireNonNull(propertySet, "propertySet cannot be null");