-
Notifications
You must be signed in to change notification settings - Fork 960
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: Use type metadata description get/update apis #1876
feat: Use type metadata description get/update apis #1876
Conversation
Signed-off-by: Kristen Armes <karmes@lyft.com>
Signed-off-by: Kristen Armes <karmes@lyft.com>
def _set_type_metadata_is_editable(type_metadata: TypeMetadata, is_editable: bool) -> None: | ||
type_metadata['is_editable'] = is_editable | ||
for tm in type_metadata['children']: | ||
_set_type_metadata_is_editable(tm, is_editable) |
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.
so right now editability is only controlled down to the parent level? Lets say we want to set a nested column to be have an editable description we can't then set one of it's sub column's to not have an editable description right?
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.
That's true as of now, I followed how the columns are handled as well, since it appears that as of now it only has table level configurations to determine whether columns are editable or not. I think this could be adjusted in the future pretty easily if that were to change
Signed-off-by: Kristen Armes <karmes@lyft.com>
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.
LGTM
typeConstant, // eslint-disable-line @typescript-eslint/no-unused-vars | ||
topLevelTmName, // eslint-disable-line @typescript-eslint/no-unused-vars |
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 you can do:
const [ columnName, _, _, ...tmNames] ...
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.
In this case I was getting the error Cannot redeclare block-scoped variable '_'
since there are two, but in any case I seem to remember from a previous PR where I had I tried the underscore for one unused var, the checks still required the no unused var disable
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 see, makes sense to complain with two of those.
Whatever works will be fine.
frontend/amundsen_application/static/js/ducks/tableMetadata/api/helpers.ts
Outdated
Show resolved
Hide resolved
frontend/amundsen_application/static/js/ducks/tableMetadata/api/v0.ts
Outdated
Show resolved
Hide resolved
frontend/amundsen_application/static/js/ducks/tableMetadata/api/v0.ts
Outdated
Show resolved
Hide resolved
[109, 6, 22, "Must use destructuring props assignment", "2225424112"], | ||
[116, 4, 24, "Must use destructuring props assignment", "3049746099"], | ||
[134, 19, 20, "Script URL is a form of eval.", "3373049033"], | ||
[146, 8, 207, "A control must be associated with a text label.", "2914986446"] |
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.
Thanks a lot!
Signed-off-by: Kristen Armes <karmes@lyft.com>
This reverts commit 08bea6c.
Signed-off-by: Kristen Armes karmes@lyft.com
Summary of Changes
Tests
ColumnDescEditableText
to test when it is used by a top level column or a nested columnDocumentation
N/A
CheckList
Make sure you have checked all steps below to ensure a timely review.