-
Notifications
You must be signed in to change notification settings - Fork 115
Fix variants on Response class - use value_body pattern #5622
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
Conversation
|
Following you can find the validation changes against the target branch for the APIs. No changes detected. You can validate these APIs yourself by using the |
flobernd
left a comment
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 adding a rule for this is not a bad thing to do! I would even go further:
Let's add a rule that explicitly checks if a variant is valid for a given type, e.g.:
Request > none
Response > none
Interface > Container
Alias > Internal, External, Untagged
...
In this particular case the compiler should also throw an exception and refuse emitting the schema.json since this creates a situation that is not mappable to the metamodel. We do this in other situations when a tag is incorrectly used, so this is probably an oversight.
Happy to merge this as is for now. We can improve later :-)
JoshMock
left a comment
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.
Code LGTM!
Does this bug warrant a linting rule? - its a rare and obvious mistake. Maybe a rule is not necessary
It caught one bug that you fixed, and is otherwise not going to create any noise. If it ends up causing us pain later we can discuss pulling it out.
Merging, then! @margaretjgu Can you please capture the improvements that Florian identified in a new spec validation issue? |
|
The backport to To backport manually, run these commands in your terminal: # Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-9.1 9.1
# Navigate to the new working tree
cd .worktrees/backport-9.1
# Create a new branch
git switch --create backport-5622-to-9.1
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 1cadc1b49fc64f0f580a6e981bfdcd6ff8389cb5
# Push it to GitHub
git push --set-upstream origin backport-5622-to-9.1
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-9.1Then, create a pull request where the |
|
The backport to To backport manually, run these commands in your terminal: # Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-9.2 9.2
# Navigate to the new working tree
cd .worktrees/backport-9.2
# Create a new branch
git switch --create backport-5622-to-9.2
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 1cadc1b49fc64f0f580a6e981bfdcd6ff8389cb5
# Push it to GitHub
git push --set-upstream origin backport-5622-to-9.2
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-9.2Then, create a pull request where the |
Created new issue #5642 |
* removes duplicated `ResponseBody` * moves body back to `Response` * removes `@variants: container` from Response
Closes #778
Original code defined response as an object with no reusable type name, and cannot put
@variant containeron this objectRefactored to use the
value_bodypattern (same as SearchResponse)Does this bug warrant a linting rule? - its a rare and obvious mistake. Maybe a rule is not necessary