[uss_qualifier/resource] Add base class for geospatial modifier#1467
[uss_qualifier/resource] Add base class for geospatial modifier#1467the-glu wants to merge 1 commit into
Conversation
|
I think I misread #1465 and would like to adjust it before looking closely at this one. The issue is that I would expect "resource modifier" to mean something that modifies resources, but the more important aspect of ResourceModifier in #1465 is that it presents as the modified resource itself, so it seems like a better name would be "IndexAdjustableResource". That's important because in this PR we have GeospatialModifierResource and with a name like that, I would expect it to handle any geospatial modification we might want to make to resources. Instead, it's actually just one particular way to offset GeospatialResources. With AdjustableResource, I think it would be more obvious that a name like GeospatiallyOffsetResource is probably more descriptive than GeospatialModifierResource. I think this is important because this is an external surface of the product that users will directly see and touch. I'll propose an adjustment PR. |
Ok, to do that (behaving as the modified resource, witch it doesn't do for now) we can just have it expose the underlying type as well + proxy with getattr()/setattr() attributes to the underling resources. Let me know, I can also do the change :) |
420725a to
5cb5a06
Compare
|
@BenjaminPelletier I pre-rebased that one on top of #1474 Not sure about naming for this base class, I will update next ones in the chain when that one seems ok |
|
This branch currently has merge conflicts which makes the diffs hard to interpret; can we merge main into this branch or rebase the changes onto main? |
5cb5a06 to
d10f877
Compare
Rebased, not sure why it diverged again |
89fbdf5 to
8f7b469
Compare
8f7b469 to
34b5e74
Compare
|
@BenjaminPelletier thanks for the review, everything should be fixed :) |
| pass | ||
|
|
||
|
|
||
| class GeospatialModifierSpecification(ImplicitDict): |
There was a problem hiding this comment.
I didn't renamed this one as it was not in the suggestion, should we as well?
Continuing on #1465 for #1458, this PR introduce:
GeospatialResource, as suggested to ease implementation ofGeospatialModifier, allowing a resource to generate unique 'Geospatial' resourcesInclude simple unit tests with a
TestSquareResource.