-
Notifications
You must be signed in to change notification settings - Fork 14.2k
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
feat: Sqllab to Explore UX improvements api changes #11836
Conversation
Codecov Report
@@ Coverage Diff @@
## master #11836 +/- ##
==========================================
+ Coverage 66.52% 67.56% +1.03%
==========================================
Files 916 916
Lines 44545 44554 +9
Branches 4227 4227
==========================================
+ Hits 29634 30101 +467
+ Misses 14781 14350 -431
+ Partials 130 103 -27
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
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.
Looks great, but I noticed a typo in the OpenAPI docstring
@@ -284,6 +289,11 @@ def put(self, pk: int) -> Response: | |||
500: | |||
$ref: '#/components/responses/500' | |||
""" | |||
override_columns = ( | |||
bool(strtobool(request.args["override_columns"])) |
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.
It makes me angry that strtobool
returns an integer. 😠
- in: path | ||
schema: | ||
type: bool | ||
name: override_column |
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.
override_columns
dataset = DatasetDAO.update( | ||
model=self._model, | ||
properties=self._properties, | ||
commit=True, |
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.
This change threw me off because commit
already defaults to True
.
SUMMARY
Breaking down #11755 to have the api change separately get checked in.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TEST PLAN
tox -e py38-sqlite -vv -- tests/datasets/api_tests.py::TestDatasetApi::test_update_dataset_item_w_override_columns
ADDITIONAL INFORMATION