truncate strings that are stored as keywords in ES (#159)#159
truncate strings that are stored as keywords in ES (#159)#159beniwohli wants to merge 1 commit intoelastic:masterfrom
Conversation
beniwohli
force-pushed
the
keyword-truncation
branch
from
babcb30 to
4961552
Compare
this should avoid schema errors for values that are too long closes elastic#156 closes elastic#159
hmdhk
left a comment
hmdhk
left a comment
There was a problem hiding this comment.
As a general issue, I'd say handle checking required fields as well, I can see that you have placeholders for some fields. but that's a separated issue.
| def keyword_field(string): | ||
| if not isinstance(string, compat.string_types) or len(string) <= KEYWORD_MAX_LENGTH: | ||
| return string | ||
| return string[:KEYWORD_MAX_LENGTH - 1] + u'…' |
There was a problem hiding this comment.
@beniwohli , Adding '…' would add 3 characters to the length which might exceed 1024 limit.
A separate issue: In my opinion '...' is not necessary, I would prefer adding a property to say the name was truncated and let the ui decide about the "ellipsis", but even that I think at this stage is not necessary.
There was a problem hiding this comment.
The character I use there is the unicode ellipsis, so it's only one character :)
I'm not sure adding an additional field for every keyword field to indicate if it was truncated or not is feasible. Having to truncate a field should be an absolute edge case.
There was a problem hiding this comment.
I see, sorry my python is a bit rusty 😄 !
It doesn't have to be a field for every keyword field. But my point was that I would prefer not adding anything to the fields that might have a meaning in different contexts and keeping what the data was originally
There was a problem hiding this comment.
I'm going to approve the changes, then you can decide about the 'ellipsis'!
| 'pid': os.getpid(), | ||
| 'argv': sys.argv, | ||
| 'title': None, | ||
| 'title': None, # Note: if we implement this, the value needs to be wrapped with keyword_field |
There was a problem hiding this comment.
I like your note to your future self 😄 , I do that as well!
beniwohli
force-pushed
the
keyword-truncation
branch
from
4961552 to
45d32c2
Compare
this should avoid schema errors for values that are too long closes elastic#156 closes elastic#159
this should avoid schema errors for values that are too long closes elastic#156 closes elastic#159
beniwohli
force-pushed
the
keyword-truncation
branch
from
45d32c2 to
38f8033
Compare
2 participants
this should avoid schema errors for values that are too long
closes #156
closes #159