Skip to content
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

crash multiline header fix #222

Merged
merged 1 commit into from
Aug 30, 2017
Merged

crash multiline header fix #222

merged 1 commit into from
Aug 30, 2017

Conversation

Taliik
Copy link
Member

@Taliik Taliik commented Aug 24, 2017

fixed the problem with the formatting if the first row was empty, in line 585 & 586
also added some formatting tests

@Taliik Taliik requested a review from mxm August 24, 2017 14:59
@codecov-io
Copy link

codecov-io commented Aug 24, 2017

Codecov Report

Merging #222 into master will increase coverage by 0.29%.
The diff coverage is 83.95%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #222      +/-   ##
==========================================
+ Coverage   82.75%   83.04%   +0.29%     
==========================================
  Files          17       17              
  Lines        2064     2165     +101     
==========================================
+ Hits         1708     1798      +90     
- Misses        356      367      +11
Impacted Files Coverage Δ
src/crate/crash/test_command.py 98.6% <100%> (+0.17%) ⬆️
src/crate/crash/tabulate.py 58.98% <77.39%> (+2.8%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f6d0303...2e476c5. Read the comment docs.

@coveralls
Copy link

coveralls commented Aug 24, 2017

Coverage Status

Coverage increased (+0.3%) to 83.048% when pulling 918a2d4 on f/crash-multiline-header-fix into f6d0303 on master.

# wcswidth and _visible_width don't count invisible characters;
# padfn doesn't need to apply another correction
if strings[0] == '':
strings[0] = ' '
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems a bit hacky. Can't we enable support for empty string padding in padfn?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these the only changes from the old multiline formatting code?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it kinda is. I don't think padfn supports empty strings, but i will have another look to see if i can find a better solution.
yes these are the only changes.

@coveralls
Copy link

coveralls commented Aug 28, 2017

Coverage Status

Coverage decreased (-0.6%) to 82.148% when pulling 38eb85f on f/crash-multiline-header-fix into f6d0303 on master.

@coveralls
Copy link

coveralls commented Aug 28, 2017

Coverage Status

Coverage increased (+0.2%) to 82.966% when pulling ebe7a41 on f/crash-multiline-header-fix into f6d0303 on master.

@coveralls
Copy link

coveralls commented Aug 30, 2017

Coverage Status

Coverage increased (+0.3%) to 83.048% when pulling 1eaac05 on f/crash-multiline-header-fix into f6d0303 on master.

CHANGES.txt Outdated
@@ -5,6 +5,8 @@ Changes for crash
Unreleased
==========

- added back support for multiline table headers
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could capitalize the first letter here.

@Taliik Taliik force-pushed the f/crash-multiline-header-fix branch from bcfbf13 to 2e476c5 Compare August 30, 2017 08:22
@coveralls
Copy link

coveralls commented Aug 30, 2017

Coverage Status

Coverage increased (+0.3%) to 83.048% when pulling 2e476c5 on f/crash-multiline-header-fix into f6d0303 on master.

@Taliik Taliik merged commit cdb775d into master Aug 30, 2017
@Taliik Taliik deleted the f/crash-multiline-header-fix branch August 30, 2017 08:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants