refactor(specs): new predict segment condition syntax #1202
Conversation
✅ Deploy Preview for api-clients-automation ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
✗ The generated branch has been deleted.If the PR has been merged, you can check the generated code on the |
|
Do not merge yet. See 'Dependencies' heading in the PR description |
Co-authored-by: Thomas Raffray <Fluf22@users.noreply.github.com>
| segmentFilterOperatorNumerical: | ||
| description: The operator used on the numerical filter value. | ||
| type: string | ||
| default: = | ||
| enum: | ||
| - = | ||
| - '!=' | ||
| - '>' | ||
| - '>=' | ||
| - < | ||
| - <= |
There was a problem hiding this comment.
not related to this PR but wouldn't you want your API to have the same convention for operators? I see that in some places you use AND/OR, here we use mathematical operators but below (in segmentFilterProbability) we use an other form
There was a problem hiding this comment.
We tried to use < and > as object keys for segmentFilterProbability but the spec didn't allow it. So here we use some aliases.
Is there a symbol operator for OR? Of course AND can be +.
There was a problem hiding this comment.
segmentFilterProbability is out of date, no?
api-clients-automation/specs/predict/responses/Segment.yml
Lines 345 to 353 in f6de2c0
There was a problem hiding this comment.
These textual representations (lt, lte etc) actually resolve as symbols <, <=. See e1c61be
Is this what you are referring to?
There was a problem hiding this comment.
I was not speaking for the generation, indeed the workaround make it work, but for the consistency of the API it might be great to consider having the same operator convention for each parameters. For example, every comparison could be made with: lt lte gt gte eq neq and or
There was a problem hiding this comment.
(btw I don't think it's a discussion for this PR it can still move on!)
This reverts commit ae39a7f.
| "name": "predictions.order_value", | ||
| "filters": [ | ||
| { | ||
| "operator": ">", |
There was a problem hiding this comment.
you'd need to update those tests
| @@ -120,10 +120,33 @@ public OperationsMap postProcessOperationsWithModels(OperationsMap objs, List<Mo | |||
|
|
|||
| @Override | |||
| public String toEnumVarName(String value, String datatype) { | |||
| if ("String".equals(datatype) && !value.matches("[A-Z0-9_]+")) { | |||
| // when it's not a string, we don't want to change the name of the variable generated | |||
There was a problem hiding this comment.
you can revert this commit if there's no more symbols!
This reverts commit b4d4849.
🧭 What and Why
Changing the Predict segment syntax from a
stringto anobjectthat follows the proposed specification.🎟 JIRA Ticket: PRED-921
https://algolia.atlassian.net/browse/DI-764
Changes included:
versionto segment objectconditionsformat to segment objectDependencies
Do not merge until the API supports the new format. Even if the PR below is merged the changes may not have been deployed to production.
https://github.com/algolia/predict-api/pull/63