[GitHub] [superset] betodealmeida commented on a diff in pull request #23729: chore: migrate `sqllab_viz` endpoint to api v1

["betodealmeida (via GitHub)" <gi...@apache.org>]

betodealmeida commented on code in PR #23729:
URL: https://github.com/apache/superset/pull/23729#discussion_r1175827419


##########
superset/datasets/schemas.py:
##########
@@ -49,14 +49,14 @@ class DatasetColumnsPutSchema(Schema):
     column_name = fields.String(required=True, validate=Length(1, 255))
     type = fields.String(allow_none=True)
     advanced_data_type = fields.String(allow_none=True, validate=Length(1, 255))
-    verbose_name = fields.String(allow_none=True, Length=(1, 1024))
+    verbose_name = fields.String(allow_none=True, metadata={Length: (1, 1024)})

Review Comment:
   I assume you're doing this because Marshmallow deprecated passing arbitrary keys as metadata, and now recommends explicitly using the `metadata` argument, ie:
   
   ```python
   fields.String(allow_none=True, description="foo")  # DEPRECATED
   fields.String(allow_none=True, metadata={"description": "foo"})  # RECOMMENDED
   ```
   
   The reason why the latter is recommended is because the former will silently swallow keywords arguments, which is exactly what's happening here; `Length()` is supposed to be a validator, not metadata. So you want:
   
   ```suggestion
       verbose_name = fields.String(allow_none=True, validate=Length(1, 1024))
   ```



##########
superset/datasets/schemas.py:
##########
@@ -71,7 +71,7 @@ class DatasetMetricsPutSchema(Schema):
     metric_name = fields.String(required=True, validate=Length(1, 255))
     metric_type = fields.String(allow_none=True, validate=Length(1, 32))
     d3format = fields.String(allow_none=True, validate=Length(1, 128))
-    verbose_name = fields.String(allow_none=True, Length=(1, 1024))
+    verbose_name = fields.String(allow_none=True, metadata={Length: (1, 1024)})

Review Comment:
   Same here, the original code is broken and you want:
   
   ```suggestion
       verbose_name = fields.String(allow_none=True, validate=Length(1, 1024))
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org