-
Notifications
You must be signed in to change notification settings - Fork 88
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
Allow UISerializer to serialize Polygon locations in addition to Points #1924
base: master
Are you sure you want to change the base?
Allow UISerializer to serialize Polygon locations in addition to Points #1924
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this! I think the tests could be simplified, but otherwise looks like a great fix to an annoying bug.
@@ -31,7 +31,7 @@ def _add_affiliation_name(creatibutors): | |||
|
|||
|
|||
@pytest.fixture(scope="function") | |||
def full_to_dict_record(full_record_to_dict): | |||
def full_to_dict_record_point(full_record_to_dict): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a fan of having two versions of this, one with points and one with polygons. I know there are some organizational issues around the test data, so others might have better ideas. But I think including both polygon and point in the full_dict_record, and then checking once that they serialize correctly would be simpler but still have the same effect in validating that polygon is working correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think including both polygon and point in the full_dict_record, and then checking once that they serialize correctly would be simpler but still have the same effect in validating that polygon is working correctly.
Great idea! I will work on that change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tmorrell I've consolidated the test cases and fixtures.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! I'll plan on merging in the next few days unless there is feedback from others.
Description
This PR updates the UI Serializer's GeometrySchema to handle Polygon features.
It addresses the bug in #1918
Checklist
Ticks in all boxes and 🟢 on all GitHub actions status checks are required to merge:
Frontend