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

fix: use appropriate methods to convert the numbers in XML #1171

Merged
merged 1 commit into from
Jun 17, 2024

Conversation

emanuelaepure10
Copy link
Contributor

Conversion of the numbers using appropriate methods of each different type.

ING-3965

Copy link
Member

@stempler stempler left a comment

Choose a reason for hiding this comment

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

See comment.

Additionally it would be great to have unit tests accompany the implementation to (1) verify it and (2) prevent future regressions.

@emanuelaepure10 emanuelaepure10 force-pushed the fix/ING-3965 branch 2 times, most recently from 2a161dd to 4b3eb6c Compare May 7, 2024 20:47
@stempler stempler self-requested a review May 8, 2024 13:23
@stempler stempler dismissed their stale review May 8, 2024 13:23

main changes were done as requested

Copy link
Member

@stempler stempler left a comment

Choose a reason for hiding this comment

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

Generally looks good to me, but I have some doubts if the added converters are sufficient.

As mentioned previously it would be great to have unit tests to accompany your implementation to (1) verify it and (2) prevent future regressions.

Related to the conversions there could be other types of values/bindings for the XML simple types, than what you implemented now.
When we load an XML schema we here use functionality from Geotools to determine the binding. As an example, for the integer simple type a BigInteger binding is used. So I think at least the respective binding types from that class need to be supported for the conversion.

@emanuelaepure10 emanuelaepure10 force-pushed the fix/ING-3965 branch 4 times, most recently from b3d6dae to f08c2bc Compare May 14, 2024 07:51
@emanuelaepure10 emanuelaepure10 force-pushed the fix/ING-3965 branch 3 times, most recently from a312768 to 5e59ecd Compare May 22, 2024 15:27
Copy link
Member

@stempler stempler left a comment

Choose a reason for hiding this comment

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

Points from my previous review comment are still open - any feedback on those?
See #1171 (review)

@emanuelaepure10
Copy link
Contributor Author

If you refer to the points about testing, yes there are yet no test done.
Would you have an example of something similar from where to get an idea on how to create these kind of test?

@stempler
Copy link
Member

If you refer to the points about testing, yes there are yet no test done.

Yes, I meant that but also the other mentioned point on the conversions, which should at least cover the conversions between the bindings we use for the XML simple types and the XmlBeans classes. The example I mentioned was that for the integer XML simple type the binding BigInteger is used.
Though it might be possible that this is covered by a transformation chain with intermediate types? If that's the case then this could be verified with the unit tests. Those should ideally include at least one test for every number related simple type.

Would you have an example of something similar from where to get an idea on how to create these kind of test?

I would propose to look at StreamGmlWriter2Test for examples of tests writing XML/GML. Though in your case for the validation you would want to check the written XML instead of loading it as instance, since you want to verify how it is represented in XML as text.

@emanuelaepure10
Copy link
Contributor Author

Yes, I meant that but also the other mentioned point on the conversions, which should at least cover the conversions between the bindings we use for the XML simple types and the XmlBeans classes. The example I mentioned was that for the integer XML simple type the binding BigInteger is used.

Why for an integer cannot be used Integer instead of BigInteger?

I would propose to look at StreamGmlWriter2Test for examples of tests writing XML/GML. Though in your case for the validation you would want to check the written XML instead of loading it as instance, since you want to verify how it is represented in XML as text.

So that means that StreamGmlWriter2Test doesn't help me.
There is anything around that would check the written XML?

@stempler
Copy link
Member

@emanuelaepure10 It would be great if you would use fixup commits while working on the PR because that makes reviews much easier.

Why for an integer cannot be used Integer instead of BigInteger?

I would guess that the reason they did that in Geotools might have been that they wanted to be able to represent numbers that are outside the range supported by integer.

So that means that StreamGmlWriter2Test doesn't help me.

Why?

You can in a similar way

  • load an XML schema
  • create test data programmatically
  • write the data

The main difference would be that you validate the written data in a different way.

There is anything around that would check the written XML?

An easy way to load and process XML is for instance the Groovy XmlSlurper class.

@emanuelaepure10
Copy link
Contributor Author

It would be great if you would use fixup commits while working on the PR because that makes reviews much easier.

I'll do so. I knew that I should always squash them.

Yes, I meant that but also the other mentioned point on the conversions, which should at least cover the conversions between the bindings we use for the XML simple types and the XmlBeans classes.

I think that all of this has been already covered.

Copy link
Member

@stempler stempler left a comment

Choose a reason for hiding this comment

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

Do you plan to still add tests?

Related to the commit message I think this is a fix, not a feature.

@emanuelaepure10 emanuelaepure10 changed the title feat: use appropriate methods to convert the numbers in XML fix: use appropriate methods to convert the numbers in XML May 31, 2024
@emanuelaepure10
Copy link
Contributor Author

@stempler
I have extended an existing test in GmlInstanceCollectionTest, as it seemed the most appropriate place from my perspective. Please review the solution and let me know if it aligns with your expectations.

Thank you.

@emanuelaepure10 emanuelaepure10 force-pushed the fix/ING-3965 branch 2 times, most recently from 74126c2 to b5bd614 Compare June 5, 2024 13:19
@emanuelaepure10 emanuelaepure10 force-pushed the fix/ING-3965 branch 2 times, most recently from 02e1dd2 to 0c119ef Compare June 10, 2024 12:50
@emanuelaepure10 emanuelaepure10 added the challenged For PRs to indicate that the implementation has been challenged label Jun 17, 2024
Conversion of the numbers using appropriate methods of each different type.

ING-3965
@emanuelaepure10 emanuelaepure10 merged commit 2baec50 into halestudio:master Jun 17, 2024
4 checks passed
@stempler stempler deleted the fix/ING-3965 branch June 17, 2024 14:08
Copy link

we-release bot commented Jun 19, 2024

🎉 This PR is included in version 5.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@we-release we-release bot added the released label Jun 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
challenged For PRs to indicate that the implementation has been challenged released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants