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

Unicode support and bugfixes #113

Merged
merged 9 commits into from
Jan 26, 2021
Merged

Conversation

nanobowers
Copy link
Contributor

@tj and @nateberkopec

I added support for unicode type tables in this PR, while trying to maintain backward compatiblity with the API and existing tests.
There are very subtle formatting differences (removal of certain intersections near colspans) that happened in order to support unicode, but other than that, existing tests should be intact.

In addition to supporting a variety of unicode border styles, this PR also fixes:

I realize this a lot to drag into one PR, but most of the fixes for the above issues somewhat naturally fell out of the change in how the internals of how styling was handled.

@nateberkopec
Copy link
Collaborator

Commenting to mark that I've seen this and will get to it this week.

@nateberkopec
Copy link
Collaborator

A few comments - also, please write up your changes in History.rdoc. Don't worry about headings, just describe the changes.

This is really great work, the amount of hours you must have put into this! Thank you so much.

I'm thinking this should probably be a major version bump due to the changes to the class hierarchies, although it looks like you managed to avoid removing or significantly changing any actual method names or APIs. What do you think?

@nanobowers
Copy link
Contributor Author

@nateberkopec , Thanks for the nice comments and reviewing such a large PR. I have addressed the unnecessary returns and updated the History.rdoc (without incrementing the version, so will need another tweak on based on the decision regarding version number)

I agree that this is a pretty big change (re: class hierarchies), and also it changes output slightly for the Ascii case, so gems that use this gem may end up breaking their test suites if they expect the old output. That said, despite minor changes to ascii output formatting, the API itself didn't break from release to release. I did consider making slightly more aggressive changes that would break backward compatibility, but since this is such an old and established gem, it didn't feel appropriate.

I also have considered adding support for a footer which would affect the Style/Border API (currently we only support a header), addressing a few more enhancement tickets and cleaning up the code a bit more.

Anyway, I'm fine with a 2.1.0 or a 3.0.0 or whatever you recommend, but if the decision is to do a major rev, I'd want to add footer support before doing so.

@nanobowers nanobowers mentioned this pull request Jan 23, 2021
@nateberkopec
Copy link
Collaborator

@nanobowers With the age of this gem, I think we should just be cautious and make it a 3.0.

Do you want to add on footer support here or can I merge?

@nanobowers
Copy link
Contributor Author

Please go ahead and merge, I'll try to submit another PR for footer support soon. I just committed updates to rev the version-number to 3.0.0 in the History.rdoc and version.rb.

@nanobowers
Copy link
Contributor Author

nanobowers commented Jan 24, 2021

@nateberkopec Actually, can you hold off on merging this for another few days? i think there's a better way to do it to make the footer /separator features more transparent.

@nateberkopec
Copy link
Collaborator

Can do 👍

…r and footer or other emphasized (strong) separators.

Removed class inheritance and logic for previous/next row and replaced with adding a non-default border_type into the table elaboration logic.

Also split the table elaboration (of implicit separators) from the rendering portion so that it can be intercepted in the case where a user would want to modify the elaborated separators.
@nanobowers
Copy link
Contributor Author

@nateberkopec I think this is a better implementation - it removed some of the embedded logic and moves it to table elaboration. As part of this change, I exposed a way for users to choose different separators, and provided some more styling options on the separators. Additional tests / documentation have been added. Apologies for the churn on this.

@nateberkopec nateberkopec merged commit 9b4e880 into tj:master Jan 26, 2021
@nateberkopec
Copy link
Collaborator

Really incredible work! This is more attention than terminal-table has gotten in years! Thank you so much @nanobowers

@nateberkopec
Copy link
Collaborator

3.0 will release tomorrow

@nateberkopec
Copy link
Collaborator

@nanobowers thanks for all the linking/reminders to close

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.

2 participants