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

Table padding option #130

Merged
merged 16 commits into from
May 29, 2016
Merged

Table padding option #130

merged 16 commits into from
May 29, 2016

Conversation

theSage21
Copy link
Collaborator

Cell padding in Tables is added as an option to maintain backward compatibility. Solves #125

@coveralls
Copy link

coveralls commented May 28, 2016

Coverage Status

Coverage increased (+0.2%) to 97.143% when pulling ff58b47 on theSage21:master into 3950277 on Alir3z4:master.

@theSage21
Copy link
Collaborator Author

@Alir3z4 Could you squash and pull this one? I am unable to do so.

@Alir3z4
Copy link
Owner

Alir3z4 commented May 28, 2016

Sure, I'll look into it and review it as well.
On May 28, 2016 1:01 PM, "arjoonn sharma" [email protected] wrote:

@Alir3z4 https://github.com/Alir3z4 Could you squash and pull this one?
I am unable to do so.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#130 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAkFCQ9NVedyOreChDOA867nDS-qvaaHks5qGASCgaJpZM4IpCBJ
.

@Changaco
Copy link

Merely looking for | in a line doesn't seem to me like a robust way to detect tables in a markdown document. I think you should wrap the tables with an HTML tag when padding is enabled, so that you'll have a clear marker to look for in the post-processing step.

max_width = [len(x.rstrip()) + right_margin for x in lines[0].split('|')]
for line in lines:
cols = [x.rstrip() for x in line.split('|')]
max_width = [max(len(x) + right_margin, old_len) for x, old_len in zip(cols, max_width)]
Copy link
Owner

Choose a reason for hiding this comment

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

pep8 please ;)
I'll be setting up some code quality checkers and linters

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm. For some reason my vim config of flake8 did not pick up any faults. Could you point out the changes I need to make?

Copy link
Owner

Choose a reason for hiding this comment

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

No problem, leave it out.
We still don't have strict rules for pep8 or code quality.
We'll be cleaning them with #131

@Alir3z4
Copy link
Owner

Alir3z4 commented May 28, 2016

@theSage21 I agree with @Changaco about tagging the output for post-processing formatting, there might be places where it will fail to find the correct | also when you have tagged, I don't think that might be much of performance.

@Alir3z4 Alir3z4 self-assigned this May 28, 2016
Header 1 | Header 2 | Header 3
----------|-----------|----------------------------------------------
Content 1 | Content 2 | ![200](http://lorempixel.com/200/200) Image!
Content 1 | Content 2 | ![200](http://lorempixel.com/200/200) Image!

Choose a reason for hiding this comment

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

I suggest varying the content of the first two columns a little so we can better see padding in action.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sounds good. I essentially copied the google_doc_tables test case and derived from there. I'm pushing this.

@coveralls
Copy link

coveralls commented May 28, 2016

Coverage Status

Coverage increased (+0.2%) to 97.143% when pulling 0a7bdb5 on theSage21:master into 3950277 on Alir3z4:master.

@coveralls
Copy link

coveralls commented May 28, 2016

Coverage Status

Coverage increased (+0.2%) to 97.163% when pulling f84b591 on theSage21:master into 3950277 on Alir3z4:master.

@theSage21
Copy link
Collaborator Author

@Alir3z4 I think almost everything is fixed now. Squash and pull now?

@Alir3z4
Copy link
Owner

Alir3z4 commented May 29, 2016

Has the read me and docs has been updated with new changes in this pull
request? Readme.md, usage.md, other docs
On May 29, 2016 11:18 AM, "arjoonn sharma" [email protected] wrote:

@Alir3z4 https://github.com/Alir3z4 I think almost everything is fixed
now. Squash and pull now?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#130 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAkFCSNvi4pTDaDc_kaITGd_BIjR04F-ks5qGT23gaJpZM4IpCBJ
.

@theSage21
Copy link
Collaborator Author

Ah, completely forgot about those. My bad. Adding them in.

@coveralls
Copy link

coveralls commented May 29, 2016

Coverage Status

Coverage increased (+0.2%) to 97.163% when pulling e1872ba on theSage21:master into 3950277 on Alir3z4:master.

@Alir3z4
Copy link
Owner

Alir3z4 commented May 29, 2016

If things are done, lemme know so I can merge in.

@theSage21
Copy link
Collaborator Author

As far as I can see, everything is done. Please Squash and merge.

@Alir3z4 Alir3z4 merged commit df1d723 into Alir3z4:master May 29, 2016
@Alir3z4
Copy link
Owner

Alir3z4 commented May 29, 2016

Great work everybody.
This is indeed very nice and beautiful.

The pull request is in the master and I'll be releasing a new release soon.

@theSage21
Copy link
Collaborator Author

Would have been nice if you would have squashed it first. I made a mess out of the commits.

@Alir3z4
Copy link
Owner

Alir3z4 commented May 29, 2016

It has been squashed already, isn't ?
On May 29, 2016 3:55 PM, "arjoonn sharma" [email protected] wrote:

Would have been nice if you would have squashed it first. I made a mess
out of the commits.


You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
#130 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAkFCdNzSPqPkaT4PDBSoOBNnxtBVyotks5qGX6mgaJpZM4IpCBJ
.

@theSage21
Copy link
Collaborator Author

Hmm, I don't know. All those dirty commits still appear in the history https://github.com/Alir3z4/html2text/commits/master

@Alir3z4
Copy link
Owner

Alir3z4 commented May 29, 2016

@theSage21 Those commits are from yesterday that you pushed directly to master or from another pull request your merged I think, this is the squahed/merged commit df1d723

@theSage21
Copy link
Collaborator Author

Hmm. Right. My mistake. I need sleep it seems.

On Sun, May 29, 2016 at 6:00 PM, Alireza Savand [email protected]
wrote:

@theSage21 https://github.com/theSage21 Those commits are from
yesterday that you pushed directly to master or from another pull request
your merged I think, this is the squahed/merged commit df1d723
df1d723


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#130 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AHVj0ZKwdb2hEwId_mv1uDByo1dry7CMks5qGYbUgaJpZM4IpCBJ
.

@Changaco
Copy link

!m @theSage21 @Alir3z4 :-)

To thank you for your work I've set up small donation pledges for you on Liberapay (a new open source recurrent donations platform I created). ;-)

@theSage21
Copy link
Collaborator Author

@Changaco appreciate it. 😄

@Alir3z4
Copy link
Owner

Alir3z4 commented May 29, 2016

@Changaco Thank you, you're a great man.
I appreciate what you do and what you have done on Liberapay. I've been following it for a while and liked how/why it started. Wanted to help on its development but wasn't familiar with the work flow (I'm a Django or Flask or Klein developer aka lazy in development).

Again thank you for your great work.

@Alir3z4
Copy link
Owner

Alir3z4 commented May 29, 2016

@theSage21 @Changaco Just released a new version:
https://pypi.python.org/pypi/html2text/2016.5.29

@Changaco
Copy link

:-)

@Alir3z4 We explain a few things about our code in the README, but it's true that we have a web framework problem. We're working on cleaning up Aspen, but you know how it is: so many things to do, so little time. ;-)

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.

4 participants