feat: CSV, Excel and JSON writer - reduce to single geometries if dat… #1173

emanuelaepure10 · 2024-05-14T21:54:31Z

…a does not contain multi geometry

CSV, Excel and JSON writer have been reduced to single geometries if data contains single geometry.

ING-3570
Closes #986

stempler · 2024-05-23T17:25:46Z

...humboldt.hale.io.csv/src/eu/esdihumboldt/hale/io/csv/writer/AbstractTableInstanceWriter.java

 		if (!headerRow.contains(propertyTypeName)) {
 			headerRow.add(propertyTypeName);
 		}
+
+		if (property instanceof DefaultGeometryProperty<?>) {


The instanceof check and the cast later should be done with the interface GeometryProperty and not the implementation class.

@stempler the property that comes in the line 242 is a GeometryProperty<?> and not a GeometryCollection.

I don't quite understand. This comment was about using GeometryProperty instead of DefaultGeometryProperty, which you also did, so what is the open point here from your view?

stempler · 2024-05-23T17:26:01Z

...humboldt.hale.io.csv/src/eu/esdihumboldt/hale/io/csv/writer/AbstractTableInstanceWriter.java

+
+			if (geometry instanceof GeometryCollection
+					&& ((GeometryCollection) geometry).getNumGeometries() == 1) {
+				property = geometryProperty.getGeometry().getGeometryN(0);


Like this the CRS information gets lost.

@stempler what it was done here was just after taking the geometry from the property, so after (GeometryProperty<?>) property. If you tell me that I need to check if the property is an instanceof GeometryCollection then that condition it will never be true.
In the InstanceToJson is another story, already in the method comes a parameter which is a GeometryProperty<?>.

I'm afraid I don't understand the context of your comment.

If you tell me that I need to check if the property is an instanceof GeometryCollection then that condition it will never be true

If you refer to my comment here

this comment was on another place in the code where the JTS geometry is handled, not the geometry property

I did not tell you anything, I just asked if there was a specific reason for the change to check for some GeometryCollection subclasses instead of the base class

...humboldt.hale.io.csv/src/eu/esdihumboldt/hale/io/csv/writer/AbstractTableInstanceWriter.java

stempler · 2024-05-24T16:00:50Z

...humboldt.hale.io.csv/src/eu/esdihumboldt/hale/io/csv/writer/AbstractTableInstanceWriter.java

+		if (geometry instanceof MultiPoint) {
+			MultiPoint multiPoint = (MultiPoint) geometry;
+			if (multiPoint.getNumGeometries() == 1) {
+				return multiPoint.getGeometryN(0);
+			}
+		}
+		else if (geometry instanceof MultiLineString) {
+			MultiLineString multiLineString = (MultiLineString) geometry;
+			if (multiLineString.getNumGeometries() == 1) {
+				return multiLineString.getGeometryN(0);
+			}
+		}
+		else if (geometry instanceof MultiPolygon) {
+			MultiPolygon multiPolygon = (MultiPolygon) geometry;
+			if (multiPolygon.getNumGeometries() == 1) {
+				return multiPolygon.getGeometryN(0);
+			}
+		}


I think the logic before with the GeometryCollection was fine, any specific reason to change it?

I haven't change that. Before I have used DefaultGeometryProperty and now GeometryProperty.

Yes, you changed it. In the original version you did it the same way as in your changes to InstanceToJson, just checking for GeometryCollection and not these three subtypes. That's why I wondered if there is a specific reason to do it like that here.

Please see the comments above regarding your question.

I have already implemented the solution you proposed but for me it is not valid. Therefore, I won't be committing it. Could you please reconsider your approach? I would greatly appreciate it.

I can't really follow you. What proposal do you mean?

I see in the current commit you changed the logic back to checking GeometryCollection instead of the three subclasses that were checked in the code related to the original comment in this thread. The only thing I did was asking about if there was a specific reason for the change to check the three subclasses instead of only GeometryCollection as you did in your first implementation.

No specific reason. In my opinion was a good approach but just using getGeometryN(0) it's enough for all the cases so I dropped the method.

...humboldt.hale.io.csv/src/eu/esdihumboldt/hale/io/csv/writer/AbstractTableInstanceWriter.java

stempler · 2024-05-27T13:18:02Z

Is the second commit intended to be a fixup commit?

Then it should start with fixup! followed by the message (first line) of the original commit.
You can also use git commit --fixup <commit-sha> to create such a commit.

stempler

Please rebase locally to combine the commits (since the PR is from a fork, the /autosquash command does not work, also the commit message of the fixup commit seems to not reference the original commit correctly)

...s/eu.esdihumboldt.hale.io.json/src/eu/esdihumboldt/hale/io/json/internal/InstanceToJson.java

…a does not contain multi geometry CSV, Excel and JSON writer have been reduced to single geometries if data contains single geometry. ING-3570 Closes halestudio#986

we-release · 2024-06-19T15:34:21Z

🎉 This PR is included in version 5.2.0 🎉

The release is available on:

v5.2.0
GitHub release

Your semantic-release bot 📦🚀

emanuelaepure10 requested a review from stempler May 14, 2024 21:54

emanuelaepure10 force-pushed the feat/ING-3570 branch 5 times, most recently from 2ec4e30 to e4336a3 Compare May 23, 2024 12:31

stempler requested changes May 23, 2024

View reviewed changes

emanuelaepure10 force-pushed the feat/ING-3570 branch from e4336a3 to 24363d5 Compare May 24, 2024 09:49

stempler reviewed May 24, 2024

View reviewed changes

emanuelaepure10 force-pushed the feat/ING-3570 branch 2 times, most recently from 71d3459 to 5f2927c Compare May 24, 2024 20:25

emanuelaepure10 force-pushed the feat/ING-3570 branch from 5f2927c to 816c59d Compare May 28, 2024 08:27

stempler approved these changes May 28, 2024

View reviewed changes

...s/eu.esdihumboldt.hale.io.json/src/eu/esdihumboldt/hale/io/json/internal/InstanceToJson.java Outdated Show resolved Hide resolved

emanuelaepure10 force-pushed the feat/ING-3570 branch 6 times, most recently from c8e554d to a544940 Compare June 3, 2024 09:29

emanuelaepure10 added the challenged For PRs to indicate that the implementation has been challenged label Jun 3, 2024

feat: CSV, Excel and JSON writer - reduce to single geometries if dat…

a3213ac

…a does not contain multi geometry CSV, Excel and JSON writer have been reduced to single geometries if data contains single geometry. ING-3570 Closes halestudio#986

emanuelaepure10 force-pushed the feat/ING-3570 branch from a544940 to a3213ac Compare June 3, 2024 10:03

emanuelaepure10 merged commit 3aa38bf into halestudio:master Jun 3, 2024
4 checks passed

we-release bot added the released label Jun 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: CSV, Excel and JSON writer - reduce to single geometries if dat… #1173

feat: CSV, Excel and JSON writer - reduce to single geometries if dat… #1173

emanuelaepure10 commented May 14, 2024 •

edited

Loading

stempler May 23, 2024

emanuelaepure10 May 27, 2024

stempler May 28, 2024

stempler May 23, 2024

emanuelaepure10 May 27, 2024

stempler May 28, 2024

stempler May 24, 2024

emanuelaepure10 May 24, 2024

stempler May 27, 2024

emanuelaepure10 May 27, 2024 •

edited

Loading

stempler May 28, 2024

emanuelaepure10 May 28, 2024

stempler commented May 27, 2024

stempler left a comment

we-release bot commented Jun 19, 2024

feat: CSV, Excel and JSON writer - reduce to single geometries if dat… #1173

feat: CSV, Excel and JSON writer - reduce to single geometries if dat… #1173

Conversation

emanuelaepure10 commented May 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emanuelaepure10 May 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stempler commented May 27, 2024

stempler left a comment

Choose a reason for hiding this comment

we-release bot commented Jun 19, 2024

emanuelaepure10 commented May 14, 2024 •

edited

Loading

emanuelaepure10 May 27, 2024 •

edited

Loading