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

add support for tables without borders #643

Merged
merged 7 commits into from
Nov 1, 2023

Conversation

gko
Copy link
Contributor

@gko gko commented Sep 22, 2023

This seems like a valid table(at least in github):

header 1|header 2
--|--
data 1|data 2

rendered:

header 1 header 2
data 1 data 2

The solution in this pull request is to wrap all lines that don't have | at the beginning or the end.

I also added range call support. It doesn't change the implementation but allows to call TableFormat while having the selection.

Copy link
Member

@alerque alerque left a comment

Choose a reason for hiding this comment

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

I'm happy to see support for table formats added, but I think we're going to need to have this gated behind a feature flag. We can discuss the merits of default on or off, but I know not all Markdown engines support this syntax and it has a high risk of false positive matches.

@gko
Copy link
Contributor Author

gko commented Sep 23, 2023

hey @alerque,

I added the flag g:vim_markdown_borderless_table and documented it.

Although I should point out that in this PR I transform the borderless table to a normal one before formatting it, so it gives the same result as before.

As for default value of the flag I'll let you decide. Right now it's disabled by default.

@gko gko requested a review from alerque September 23, 2023 08:55
@gko gko force-pushed the tables_without_borders branch from 1d3e020 to afd2110 Compare November 1, 2023 13:57
Copy link
Member

@alerque alerque left a comment

Choose a reason for hiding this comment

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

Oooo, now we're talking. Gated behind a default-off flag sounds better too me since the worry is with false positives messing people up when they aren't expecting this. Thanks especially for adding tests and docs!

@alerque
Copy link
Member

alerque commented Nov 1, 2023

Would you like me to squash merge this or would you like to go through and split this up a little different? The current commit list isn't going to be very informative to a user updating through their plugin manager. One squashed feat: ... commit would be okay, or you can rebase (or I would use git revise) to spit out a feature commit for the range option and another for all the new tooling, then a doc commit for the help messages and a test one for the tests.

@gko
Copy link
Contributor Author

gko commented Nov 1, 2023

Would you like me to squash merge this or would you like to go through and split this up a little different?

I think we can squash it if it's easier.

Thank you for maintaining this plugin ❤️

P.S. tested it on some complicated tables and it worked better than prettier

@alerque alerque merged commit 46add6c into preservim:master Nov 1, 2023
3 checks passed
@alerque
Copy link
Member

alerque commented Nov 1, 2023

@gka I'm not really maintaining this, I'm just around the org to facilitate community contributions actually landing. Feel free to jump in!

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