New property structure #66

ZENEGIX · 2018-09-03T18:06:20Z

I am change structure of property.
Now we can use a 4 default proeprty (common, array, list, map) and any type of property (string, integer, enum, bean, etc.)

For example, if we want to create string array property:

import static ch.jalu.configme.properties.PropertyBuilder.*;
import static ch.jalu.configme.properties.types.PropertyType.*;

public static final Property<String[]> STRING_ARRAY = arrayProperty(stringType())
    .path("string_array")
    .defaultValue("it is default text", "you can modify it c:")
    .build();

And use it:

String[] array = settingsManager.getProperty(STRING_ARRAY);

Or you want use map? It is simple c:

public static final Property<Map<String, TimeUnit>> TIMEUNIT_MAP = mapProperty(enumProperty(TimeUnit.class))
    .path("timeunit_map")
    .defaultEntry("seconds", TimeUnit.SECONDS)
    .defaultEntry("minutes", TimeUnit.MINUTES)
    .build();

[...]

Map<String, TimeUnit> map = settingsManager.getProperty(TIMEUNIT_MAP);

You can use any properties type. Or you can create a new property type ^-^
And I change a version to 3.0, because previous version was a 2.*. I think, it is continuation this project c:

UPD: Sorry me, I am did not write a JavaDoc, because my english is bad. I hope, you write JavaDoc yourself.

I am change structure of property. Added PropertyType. Now we can use a 4 default proeprty (common, array, list, map) and any type of property (string, integer, enum, bean, etc.) And I change a version to 3.0, because previous version was a 2.*. I think, it is continuation this project c:

ljacqu · 2018-09-03T19:26:33Z

Wow, this is awesome! I want to look through it in detail tomorrow but it looks very promising and it's a nice concept.
Pertaining to the version, 1.0 is the current development version which hasn't been released yet (last release was 0.4.1) so I'd suggest to keep with the 1.0 version :)

ZENEGIX · 2018-09-03T19:31:58Z

Oh, sorry, I didn't notice, that previous version was a 0.*

Add a cache for enum and bean property types Small fix Comments for some places

coveralls · 2018-09-04T13:58:23Z

Coverage decreased (-6.05%) to 93.308% when pulling 48a7c85 on ZENEGIX:master into b22e6b0 on AuthMe:master.

ljacqu

Review time :) Comments are throughout the code. Feel free to challenge anything you don't agree with.

Just again to say, in overall I love this and you've managed to introduce property types in a reusable way, which I love.

Scheduling -- you've changed the POM version to 1.1; I agree with this. I'd like to release 1.0 before these changes. I don't have any changes planned for 1.0 anymore, so probably I'll release 1.0 this weekend. Until then we can either merge to a separate branch if you'd like, or we just keep the PR open for another week.

Test coverage -- as the @coveralls bot commented the test coverage went down by almost 19%. Test coverage is really important for me as it protects us from regressions at a later date. For example during the refactoring of 1.0 the tests were immensely helpful.
The lack of test is not a blocker for merging. I'll also create some unit tests myself, once 1.0 is released.

Introduced types -- do you actually need arrays and floats? I don't mind their addition but why is it interesting to have array properties if you can have lists; same for float vs. double? (I commented this in the code but these are important questions to me.)

ljacqu · 2018-09-04T16:40:06Z

src/main/java/ch/jalu/configme/properties/ArrayProperty.java

+        super(path, defaultValue);
+
+        if (type instanceof BeanPropertyType<?>) {
+            throw new IllegalArgumentException("BeanPropertyType not support for array property (maybe, temporarily)");


Is this out of intention (was there a problem?) or just a precaution? Is it because BeanPropertyType#convert is not supported?

"Is it because BeanPropertyType#convert is not supported?" - Yes,I could not come up with the implementation of the conversion, and I hope this can be done by you

OK, after I do the changes to Mapper we should be good to go :)

ljacqu · 2018-09-04T16:41:03Z

src/main/java/ch/jalu/configme/properties/ArrayProperty.java

+
+        // If target type is String and object is string, then return splitted string.
+        if (this.type.getType() == String.class && object instanceof String) {
+            return (T[]) ((String) object).split("\\\\n");


I really don't like this special case. Could we not have that as a separate property type if this is somehow a requirement? IMHO it would make more sense that the generic array property type behaves always the same way; that's also what I would expect as a user.

I have not thought about it yet, but I think together we will be able to come up with the best option c:

Hopefully you are OK to remove the special case. As mentioned, if needed a separate property type could be added. :)

ljacqu · 2018-09-04T16:42:04Z

src/main/java/ch/jalu/configme/properties/ArrayProperty.java

+            array[i] = this.type.toExportValue(value[i]);
+        }
+
+        return array;


In theory we could return a collection as export value and then the property resource doesn't have to be changed to support arrays. This keeps the contract of what is allowed as export values slim.

Sorry, but I do not understand what you're talking about :c

This is in the method toExportValue and its return value is Object, but actually the PropertyResource expects that the return value be either a String, a Number, or lists/maps with those types. You had to extend the PropertyResource to also support arrays, because in this implementation toExportValue returns an array. You could have avoided this by returning a list from this method instead.
For me this is not an issue; it's just an observation. It's probably a nice addition that toExportValue can return arrays, also :) So this is really not a problem.

ljacqu · 2018-09-04T16:43:10Z

src/main/java/ch/jalu/configme/properties/CommonProperty.java

+
+import javax.annotation.Nullable;
+
+public class CommonProperty<T> extends BaseProperty<T> {


I understand why both CommonProperty and BaseProperty are necessary. Hopefully later on we can find names that are more different from each other (maybe something with "type" in this class' name). But that's for the future. :)

We can rename CommonProperty to SignletonProperty, and BaseProperty to AbstractProperty, for example

Let's think of names for a while before doing any renamings :)

ljacqu · 2018-09-04T16:43:52Z

src/main/java/ch/jalu/configme/properties/ListProperty.java

+
+        // If raw list is null, then return default value
+        if (rawList == null) {
+            return this.getDefaultValue();


This method should return null if the value cannot be constructed from the reader.

Excuse me. I will keep this in mind in the future

No problem!

ljacqu · 2018-09-04T16:55:34Z

src/main/java/ch/jalu/configme/properties/types/PropertyType.java

+        return EnumPropertyType.of(type);
+    }
+
+    static FloatPropertyType floatType() {


I have some issues with the visibility of members, like these being package-private here (wouldn't we want public?) but that's small stuff we can visit later on.

ljacqu · 2018-09-04T16:56:13Z

src/main/java/ch/jalu/configme/properties/types/IntegerPropertyType.java

+
+import ch.jalu.configme.resource.PropertyReader;
+
+public class IntegerPropertyType implements PropertyType<Integer> {


Hopefully in the future we can replace a lot of the existing property implementations with these property types. This doesn't have to be done in the scope of this PR but it would be a goal of 1.1.

ljacqu · 2018-09-04T16:57:01Z

src/main/java/ch/jalu/configme/properties/types/FloatPropertyType.java

+
+import ch.jalu.configme.resource.PropertyReader;
+
+public class FloatPropertyType implements PropertyType<Float> {


Do we need float, when we have double? Kind of sad to see property reader receive a new getFloat method (= more work a user needs to implement to create a new property reader)

Maybe, another users want to use float, why not?
I am not use it, but SneakyYML allows to do this
Maybe, we can make NumberPropertyType implements PropertyType<Number> for Double, Float, Short and Byte values

For me everything is OK 👍 . I just wanted to make sure float was introduced intentionally.

ljacqu · 2018-09-04T17:04:27Z

src/main/java/ch/jalu/configme/properties/types/PropertyType.java

+
+    T convert(Object object);
+
+    Class<T> getType();


If we dropped the array property type we'd be able to remove this method :P Or the array property type could require a Class object on it directly?

Yes, getType() using is only in ArrayProperty

ljacqu · 2018-09-04T17:07:27Z

src/main/java/ch/jalu/configme/properties/ArrayProperty.java

+import java.util.Collection;
+import java.util.List;
+
+public class ArrayProperty<T> extends BaseProperty<T[]> {


Do we actually need an array type? When is it interesting to use this vs. a List<T> property?

I am use array in next case:
player.sendMessage(settingsManager.getProperty(ARRAY_PROPERTY))
Player have a methods sendMessage(String message) and sendMessage(String[] message)

OK for now :) I wanted to make sure that the array type is introduced for a reason; this is a good reason.

ljacqu · 2018-09-04T17:13:19Z

src/main/java/ch/jalu/configme/properties/types/BeanPropertyType.java

+
+    @Override
+    public B convert(Object object) {
+        throw new UnsupportedOperationException();


We should probably change the mapper to expect an Object as argument and not know about the reader. This is a change I can still do in 1.0 so we avoid a breaking change in 1.1.

Please do it yourself, because I am not read packet beanmapper

Yes, I'll do it today~ ✨

Done on master with b22e6b0

- In preparation for generic property types in 1.1 (#66), change the Mapper to no longer be aware of the property resource, but to take an Object directly that it attempts to convert to a bean - Remove path from root context in mappingContext: a root context now always has empty string as its path (i.e. paths are now relative to the mapping)

ZENEGIX · 2018-09-05T17:45:50Z

Well, I took into account all your remarks and changed some points.
In general, it remains for us to resolve several issues in order to arrive at the best option. In particular, the clearer class names and the feasibility of some possibilities.

ZENEGIX · 2018-09-05T17:47:28Z

And sorry me, sometimes I accidentally do things, for example, I place space in Mapper :c

ZENEGIX · 2018-09-06T16:58:32Z

As you asked, I changed some points in the ArrayProperty.
Now when creating ArrayProperty, we can indicate ConvertHelper, which will convert the string into an array of objects, and will correctly store the data in a config.

Example of use PrimitiveConvertHelper.DEFAULT_STRING:

public static final Property<String[]> MESSAGE = PropertyBuilder.arrayProperty(PropertyType.stringType())
    .path("message")
    .convertHelper(PrimitiveConvertHelper.DEFAULT_STRING)
    .defaultValue("hello, my friend!", "it is example message")
    .build();

The following will be created in the config file::

message: |-
  hello, my friend!
  it is example message

However, we can also use these options:

message: hello, my friend\nit is example message

message:
- hello, my friend!
- it is example message

The same can be done with numbers:

numbers: 1, 2, 666

numbers:
- 1
- 2
- 666

ZENEGIX · 2018-09-06T17:07:00Z

I also added support for relative property.

For example, we have a next config:

groups:
  player:
    prefix: '[player]'
  vip: 
    prefix: '[vip]'

Now, we can create a property with path 'prefix' and do this:

Property<String> PREFIX = [...]; //create the property with path 'prefix'

public String getPrefixForGroup(String group) {
  return settingsManager.getRelativeProperty("groups." + group, PREFIX);
}

However, when creating SettingsManager, we should not specify relative property, as standard values do not need to fit into our config.

ZENEGIX · 2018-09-06T18:52:16Z

Yay! +12% coverage!
UPD: +5%

ljacqu · 2018-09-08T07:39:57Z

This is great news! Thanks for everything. I'm going to merge to a 1.1 development branch now. A lot of new stuff has been added since — stuff we haven't discussed and stuff I'm not sure I'd want ConfigMe to include (replacements, relative properties).
It could be that I revert this on the 1.1 branch and if you need these features maybe it will be better if you have those changes locally in your application using ConfigMe, or keep your own kind of version. But this is all stuff for the future ;)

This weekend I will release 1.0 and then all the changes of the 1.1 branch will go into master and it will be the current development version. 😉

ZENEGIX added 2 commits September 3, 2018 20:39

Small fix

149293c

ZENEGIX added 4 commits September 4, 2018 14:30

Add a cache for enum and bean property types

4ed1f75

Add a cache for enum and bean property types Small fix Comments for some places

Fix

4db2044

Merge remote-tracking branch 'upstream/master'

63cbb92

Version up

fb378c8

ljacqu reviewed Sep 4, 2018

View reviewed changes

ljacqu added this to the 1.1 Release milestone Sep 4, 2018

ZENEGIX added 3 commits September 5, 2018 20:15

Add replacements for SettingsManager::getProperty

15965f9

Merge remote-tracking branch 'upstream/master'

4faf2f9

Fix

7b02009

Add a relative properties and inline convert helpers

15d28f1

Add some tests and small fix

0925e7d

New tests and small fix c:

48a7c85

ljacqu changed the base branch from master to configme-1.1-dev September 8, 2018 07:40

ljacqu merged commit 779fa79 into AuthMe:configme-1.1-dev Sep 8, 2018


		import javax.annotation.Nullable;

		public class CommonProperty<T> extends BaseProperty<T> {


		import ch.jalu.configme.resource.PropertyReader;

		public class IntegerPropertyType implements PropertyType<Integer> {


		import ch.jalu.configme.resource.PropertyReader;

		public class FloatPropertyType implements PropertyType<Float> {

New property structure #66

New property structure #66

Uh oh!

Conversation

ZENEGIX commented Sep 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ljacqu commented Sep 3, 2018

Uh oh!

ZENEGIX commented Sep 3, 2018

Uh oh!

coveralls commented Sep 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ljacqu left a comment

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!

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!

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

ZENEGIX commented Sep 5, 2018

Uh oh!

ZENEGIX commented Sep 5, 2018

ZENEGIX commented Sep 3, 2018 •

edited

Loading

coveralls commented Sep 4, 2018 •

edited

Loading

ZENEGIX commented Sep 6, 2018 •

edited

Loading

ZENEGIX commented Sep 6, 2018 •

edited

Loading