Skip to content

Conversation

thowimmer
Copy link
Contributor

@thowimmer thowimmer commented Nov 22, 2019

Hi all,

I added support for constraints on nested fields.
The support has been implemented by simply choosing the last JSON path element as the bean property name:
someNestedObject.someField -> someField = bean property name
someField -> someField = bean property name

@coveralls
Copy link

coveralls commented Nov 22, 2019

Coverage Status

Coverage increased (+0.004%) to 92.011% when pulling 2dee63d on thowimmer:nested-constraints into 9138472 on ePages-de:master.

@ozscheyge
Copy link
Contributor

ozscheyge commented Nov 22, 2019

Hey @thowimmer ,

thank you for the contribution!

As far as I can see, this was already possible before using the withMappedPath method in an explicit fashion, e.g.

withMappedPath("pathTo.my.nestedPropertyWithConstraints", "nestedPropertyWithConstraints")

This is how we are using it so far. Your change certainly makes this more convenient!
However, I'm uncertain whether there could be cases, where this behavior isn't desired

@ozscheyge
Copy link
Contributor

ozscheyge commented Nov 22, 2019

Reading the docs of substringAfter https://kotlinlang.org/api/latest/jvm/stdlib/kotlin.text/substring-after.html your implementation would only work for one nesting level:

path --> beanPropertyName
---

one.level --> level
two.levels.areTooMany --> levels.areTooMany

@thowimmer
Copy link
Contributor Author

Hi @ozscheyge,

As far as I can see, this was already possible before using the withMappedPath method in an explicit fashion, e.g.

Yes I actually saw that. I was not sure whether the visibility of the other methods is itentional. But I guess that's the case after having this conversation:-)

This is how we are using it so far. Your change certainly makes this more convenient!
However, I'm uncertain whether there could be cases, where this behavior isn't desired

I totally agree. I was expecting this method to behave like I implemented it in this PR. However - you judge whether this is what you want or not. I'm also fine to use your proposed solution with the withMappedPath method.

Reading the docs of substringAfter your implementation would only work for one nesting level

Yes good catch ! Thanks :-) I will refactor it to substringAfterLast.

I leave it up to you whether you'd like this feature to be merged or not :-)

@ozscheyge ozscheyge added the enhancement New feature or request label Nov 22, 2019
@ozscheyge
Copy link
Contributor

I'd leave it open for further feedback/opinions. If there are no objections, it should be integrated!

Copy link
Contributor

@mduesterhoeft mduesterhoeft left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution. LGTM

@ozscheyge ozscheyge merged commit 519bcf6 into ePages-de:master Nov 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants