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 new lint rule: align table columns #1232

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

Ooopz
Copy link
Contributor

@Ooopz Ooopz commented Nov 26, 2024

new lint rule, test locally several times, and I have no idea why unit test failed, maybe you can fix it.

@pjkaufman
Copy link
Collaborator

Hey @Ooopz . Thanks for taking the time to put in a PR. If I read correctly what the UT is talking about, there is a missing newline in the expected string.

I am a little hesitant to merge this on the basis that I am not sure if this works on CJK, Monospace, and non monospace fonts. I am not sure what the expectations are here, but often times the alignment of table columns fails one or more of these scenarios.

@Ooopz
Copy link
Contributor Author

Ooopz commented Nov 27, 2024

In the align logic , the lengths of Wide and Fullwidth characters (including CJK characters) are set to 2, while the lengths of other characters are set to 1. So in some monospaced fonts, which widths ratio of English characters to CJK characters is 1:2, the alignment results are good. In non monospaced fonts and some monospaced fonts widths ratio not be 1:2, the final rendered results are not perfect but acceptable. However, the final display effect depends on the fonts used, and I think in terms of code we can't do any further.

@Ooopz
Copy link
Contributor Author

Ooopz commented Nov 27, 2024

Perhaps a reminder can be added in the settings that the alignment may not perform perfectly under non-monospaced fonts?

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