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

DATAES-415 - More Field Types Support #193

Merged
merged 4 commits into from Jul 4, 2019

Conversation

Projects
None yet
2 participants
@zzt93
Copy link
Contributor

commented Nov 17, 2017

  • You have read the Spring Data contribution guidelines.
  • There is a ticket in the bug tracker for the project in our JIRA.
  • You use the code formatters provided here and have them applied to your changes. Don’t submit any formatting related changes.
  • You submit test cases (unit or integration tests) that back your changes.
  • You added yourself as author in the headers of the classes you touched. Amend the date range in the Apache license header if needed. For new types, add the license header (copy from another file and set the current year only).

@xhaggi xhaggi changed the title DATAES-415 More Field Types Supprot. DATAES-415 - More Field Types Support May 9, 2018

@odrotbohm odrotbohm force-pushed the spring-projects:master branch from 020b5e1 to ba3eba5 Jun 18, 2018

@mp911de mp911de force-pushed the spring-projects:master branch from 01cda35 to b6fa4c8 May 6, 2019

@sothawo
Copy link
Collaborator

left a comment

sorry that this PR wasn't processed earlier, this project was quite dormant for some time.
Ccould you please check my remarks?

@zzt93 zzt93 force-pushed the zzt93:moreType branch from a1aa6f6 to f8ff79d Jun 29, 2019

@zzt93 zzt93 force-pushed the zzt93:moreType branch from f8ff79d to 3f46012 Jun 29, 2019

@sothawo

This comment has been minimized.

Copy link
Collaborator

commented Jun 29, 2019

I just wrote a small test before merging this and noticed that a field with type scaled_float needs to have an attribute scaling_factor. I think it would be more appropriate to have an annotation@ScaledFloat(scaling_factor = XXX) and adapt the MappingBuilder to process this annotation.
You can either implement it in this ticket, or remove the addition of scaled_float here and I put that in a new ticket.

@zzt93

This comment has been minimized.

Copy link
Contributor Author

commented Jul 1, 2019

@sothawo What about add another field in Field called 'scalingFactor', just like what done in Date type?

@sothawo

This comment has been minimized.

Copy link
Collaborator

commented Jul 1, 2019

Basically I would prefer to have a custom annotation which just has the scaling_factor as parameter. But you are right, the @Field annotation already has some custom attributes that are applied when the type is FieldType.Date, so to be consequent that would mean, that we had to introduce a custom @DateField annotation as well, and that would be a breaking API change.

So I think it is ok to add the scaling_factor attribute to the @Field annotation - Java Type Integer - , and in the MappingBuilder.addFieldMappingParameters method process this additional attribute in case of a scaled_float field (must not be null, greater than or equal 1).

In the MappingBuilderTests class I'd add the field to the AbstractInheritedEntity class and then check the mapping in the shouldBuildMappingWithSuperclass method.

About the tests I did: I changed the FieldTypeclass according to your PR (was only 4 lines) and then added fields with the new types to the Book class of the MappingBuilderTest. That broke some test because of the mising scaling_factor attribute.

@sothawo

sothawo approved these changes Jul 4, 2019

@sothawo sothawo merged commit 8dd2f47 into spring-projects:master Jul 4, 2019

1 check passed

ci/pivotal-cla Thank you for signing the Contributor License Agreement!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.