Update TableConfigCollection to add GemdQuery config generator#967
Update TableConfigCollection to add GemdQuery config generator#967
Conversation
| lower = properties.Float('lower') | ||
| upper = properties.Float('upper') | ||
| lower = properties.Integer('lower') | ||
| upper = properties.Integer('upper') |
There was a problem hiding this comment.
I was taking a look at the AllIntergerFilter case class in the backend, and it looks like it has a field called inclusive which defaults to true. Was that omission on purpose? I wonder if it is the case that it doesn't add value.
There was a problem hiding this comment.
I'd missed that. I'll add it here as well. 🙇
| are expressed as Bounds. Attributes are expressed with links. The attribute that the | ||
| variable is being set to may be the target of a constraint as well. | ||
| type_selector: DataObjectTypeSelector | ||
| strategy for selecting data object types to consider when matching, defaults to PREFER_RUN |
There was a problem hiding this comment.
Just double-checking, Here we are not mapping attribute_template_type, I do see it in the openapi spec, is this because we are not going to use it on the SDK?
There was a problem hiding this comment.
It is in the openapi spec, but that field is not populated by any of the existing Variable objects in the SDK. For the life of me, I don't see what impact that field is supposed to have, based on digging through platform-backed.
| List of links to the material runs to use as terminal materials in the preview | ||
|
|
||
| """ | ||
| path = path = format_escaped_url( |
| register_config=True | ||
| ) | ||
| assert session.num_calls == len(query['datasets']) + 1 + 1 | ||
| assert generated != TableConfig.build(config) # Because it has ids |
There was a problem hiding this comment.
Is there any code change that can be introduced to falsify this assertion? I'm just curious about why you are validating it. It might be obvious but I'm not super familiar with this.
There was a problem hiding this comment.
The table config returned by the registration route augments the table config returned by the from_query route by adding registration metadata to the object (id, version number...). This is making sure that when you flip the register_config flag, you get the return of the register route, not the from_query route.
In theory, this test could be rewritten as The table config has meta data, and if I ignore it, then they are the same but I don't think that adds meaningful additional coverage.
pacdaemon
left a comment
pacdaemon
left a comment
There was a problem hiding this comment.
I left a few comments, but other than that it looks good. If there is a need for addressing any of the concerns feel free to ping me for a second round, otherwise feel free to merge it.
| attribute_constraints = properties.Optional( | ||
| properties.List( |
There was a problem hiding this comment.
Are null and [] actually treated differently by the backend? If not, I'd suggest there's no need to make this Optional.
There was a problem hiding this comment.
I don't disagree, but every existing Variable in the SDK uses this pattern.
| lower = properties.Float('lower') | ||
| upper = properties.Float('upper') | ||
| lower = properties.Integer('lower') | ||
| upper = properties.Integer('upper') |
There was a problem hiding this comment.
You know my thoughts on this from our argument two weeks ago: changing types is an interface break. This is a mild one, as I can't think of a standard operation it would break, since Python's standard division always returns a float, and I'm unsure why a user would do something like isinstance(value, float). But it nonetheless gets into the business of assuming which interface breaks the user cares about, which is a Bad Idea.
I've no intention of wasting more energy on such a petty problem, but I also won't be approving this PR as long as it's left in. Seeing as there are other approvers, and you've already got one from Pablo, it won't stand in your way.
There was a problem hiding this comment.
I have reverted this fix, as it was tangential to the actual goal of the PR.
In this case, this is not a breaking change because the GemdQuery API is fundamentally broken and does not support any filters, so no user workflow could actually depend on this behavior.
3 participants
Citrine Python PR
Description
This is a sub-element for supporting https://github.com/CitrineInformatics/platform-backend/pull/4731. This implements existing aspects of the API, and corrects some minor inconsistencies in the existing GemdQuery implementation.
PR Type:
Adherence to team decisions