Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Dryrun response #283
Dryrun response #283
Changes from 36 commits
7b62bab
6785baa
6bb2533
1e4990f
598a5b6
062ce94
072e947
508025b
0d2f81c
4ce2a14
8815d72
0fa8d8e
dd1b093
31d49f1
d1083ce
c010e75
efb281e
33bb685
5e2ef10
1e35187
edd8328
69a891a
357e1c6
0e036ec
89182e8
c01d415
07eca10
ede274a
5287739
eaebf36
f661cca
fa9211a
83dca7e
2972067
3dfe4c4
48ffbda
a81d5b8
8190e2b
0d2620c
e9022c1
bd2a723
ed3605a
0185f87
5181e2d
8b54442
78aa402
8ded7ec
371875f
846e7e9
5efa115
b7fb15c
d336364
8e562b9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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: can we keep this in alphabetical order?
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 like the column width is really
3 + max_width
. Is that the intention? Consistency across the API's trumps any other issues here.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.
All of the SDKs have the same type of logic where we truncate at the max width, then add the 3 period ellipses. I went back and forth on this but I think having max width describe where we truncate the real value then adding ellipses to denote that the value is truncated is the right way to go. It is tough though so if you feel strongly I can change it.
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.
No worries. Since users have an option to specify no
max_width
at all, this really isn't a big enough deal to delay the major improvements that all this makes possible.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.
There is a weird edge case here when
max_width < len(s) < max_with + 3
. You get less data and more space usage.So maybe it would be better to have the condition be:
But that could break the test, unfortunately.
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.
Maybe I should change the flag from max_width to max_value_width to prevent confusion on this?
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.
Seems like a good compromise