BUG: fix for merge page with annotation #3467 #3532
Conversation
HSY-999
changed the title
There was a problem hiding this comment.
Please have a look at the test failures and add appropriate tests.
Additionally, while your current approach might work for the current cases, I see some limitations/aspects to discuss:
- What happens to keyword-only arguments?
- What happens to unmatched keys?
- What happens to keys which have to be renamed for some reasons, for example because they conflict with an existing name (like a builtin)?
Please provide some details on how you think about this and what your proposals for mitigating them are.
| super().__init__(**kwargs) | ||
| if len(vertices) == 0: | ||
| raise ValueError("A polygon needs at least 1 vertex with two coordinates") | ||
| if len(vertices) == 0 or len(vertices) % 2 != 0: |
There was a problem hiding this comment.
How does this new check relate to your changes and why does the error message need a rewording?
There was a problem hiding this comment.
there exists a case where an ArrayObject could be passed with an odd number of positions. len(vertices) % 2 checks for an even number of positions.
i modified the error message to be more precise to the case i was considering.
| for x, y in vertices: | ||
| coord_list.append(NumberObject(x)) | ||
| coord_list.append(NumberObject(y)) | ||
| if type(vertices) is ArrayObject: |
There was a problem hiding this comment.
Why not isinstance(vertices, ArrayObject)?
There was a problem hiding this comment.
you have taught me something about python 😃
| if type(vertices) is ArrayObject: | ||
| import itertools | ||
| coord_list = vertices | ||
| vertices = [vertex for vertex in itertools.batched(vertices, 2)] |
There was a problem hiding this comment.
itertools.batched requires Python >= 3.12, while we have to support Python >= 3.9.
| super().__init__(**kwargs) | ||
| if len(vertices) == 0: | ||
| raise ValueError("A polygon needs at least 1 vertex with two coordinates") | ||
| if len(vertices) == 0 or len(vertices) % 2 != 0: |
There was a problem hiding this comment.
Please avoid unnecessary repetition by extracting common functionality.
There was a problem hiding this comment.
PolyLine could be considered a parent class of Polygon. i could also make a abstract class for these two if that is better.
both implementations we can remove repetition
it does not work in a small example, but inspect does support matching on keyword arguments in a different
they are not passed along to
good point. i will think on this. i am thinking of having some internal value in |
2 participants
see #3467
there are a couple different solutions. this is the one i made.
for my solution, i have to assume that the key name inside
DictionaryObject==__init__parameter names. for example/Verticesandvertices. with these changes the example in #3467 exits normally.i had to modify the
__init__logic forPolygonandPolyLine, as they both expectedtuples which is not the format they are stored inDictionaryObjectif we agree this is acceptable solution, i will also add test case adapted from the issue.