-
Notifications
You must be signed in to change notification settings - Fork 79
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
Remove __lilac__ prefix for signals. #164
Conversation
src/webClientLib/lilac.spec.ts
Outdated
propertyA: lilacItem('valueA', { | ||
propertyA: { | ||
text_statistics: { | ||
num_characters: 100 |
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.
nit lilacItem()
?
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.
Nice catch, done!
*/ | ||
export function isSignalField(field: LilacSchemaField): boolean { | ||
return field.path[0] === LILAC_COLUMN; | ||
export function isSignalField( |
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.
Added a comment in your optimization bug that we can instead store parent reference in the field, and avoid extra parameter. But we can do that in seperate pr
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 we should probably just walk downwards to 1) keep parity with python side and 2) keep json-serializability of this (since there's a circular reference), important for console.log() from unit tests
Spoke with Daniel offline, he said LGTM as he's on phone. Submitting! |
if len(field_keys) != 2: | ||
raise ValueError('Expected exactly two fields in signal manifest, ' | ||
f'the row UUID and root this signal is enriching. Got {field_keys}.') | ||
return next(filter(lambda field: field != UUID_COLUMN, manifest.data_schema.fields.keys())) |
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.
might be cleaner to just use manifest.enriched_path[0]
Everything back to working:
Fixes #149