Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Keep reference to type of SimpleNode value #9

Merged
merged 2 commits into from
Jun 7, 2022
Merged

Keep reference to type of SimpleNode value #9

merged 2 commits into from
Jun 7, 2022

Conversation

pfroud
Copy link
Contributor

@pfroud pfroud commented Jun 4, 2022

This is for my idea to add different colored icons for different data types, mentioned in issue #8.

The value field in SimpleNode.java is an Object, so when it is null we cannot tell what type it is. This PR adds field Class classOfValue.

KSY to easily get a null value:

meta:
  id: null_enum_test
seq:
  - id: null_enum
    enum: my_enum
enums:
   my_enum:
    1: one

Screenshot:
image

The code in ListNode.java is kind of gross. We can't check the generic type of an ArrayList at runtime because of type erasure. So, I look for a non-null element in the List and check its type.

@pfroud pfroud mentioned this pull request Jun 4, 2022
Copy link
Owner

@Mingun Mingun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that class of object need to store . I'm not thrilled to have to figure out the type of list item by specific objects, there may still be another way. Could you investigate this?

}

@Override
public Object getValue() { return value; }

public Class<?> getClassOfValue() { return classOfValue; }
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public Class<?> getClassOfValue() { return classOfValue; }
public Class<?> getValueClass() { return valueClass; }

I bet, that this name is better.

Comment on lines 40 to 41
/** Replace value.getClass() when value==null. */
private final Class<?> classOfValue;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/** Replace value.getClass() when value==null. */
private final Class<?> classOfValue;
/** Class of property, represented by the getter return value. */
private final Class<?> valueClass;

value.getClass() is slightly incorrect, because it is dynamic type, but in fact the static type is stored here. For example ArrayList vs List

if (nonNullElement.isPresent()) {
valueClass = nonNullElement.get().getClass();
} else {
valueClass = null;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is better to avoid null in that field:

Suggested change
valueClass = null;
valueClass = Object.class;

Comment on lines 104 to 105
final Optional<?> nonNullElement = value.stream().filter(Objects::nonNull).findAny();
if (nonNullElement.isPresent()) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really like the idea of looking at the content, because the type will depend on the specific object. Probably it is possible to extract some type from generic boundaries? I remember, that this should be possible.

@pfroud
Copy link
Contributor Author

pfroud commented Jun 6, 2022

Regarding ListNode.java, I agree, the way I did it is dissatisfying.

I read some StackOverflow. Type erasure does apply here, if you do:

new ArrayList<Integer>().getClass().getTypeParameters()[0].getBounds()

it just returns:

[class java.lang.Object]

But we can do what we want using the Field or Method. Overview:

  1. Call myField.getGenericType() or myMethod.getGenericReturnType()
  2. Cast to java.lang.reflect.ParameterizedType
  3. Call getActualTypeArguments()[0]

Via https://stackoverflow.com/a/1942680/7376577

I will push some more changes to this branch

@pfroud pfroud requested a review from Mingun June 7, 2022 00:58
@Mingun Mingun merged commit 664ad05 into Mingun:master Jun 7, 2022
@Mingun
Copy link
Owner

Mingun commented Jun 7, 2022

Thanks!

@pfroud pfroud deleted the class-of-null-value branch June 7, 2022 19:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants