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

Fix counting of current line-length #121

Merged
merged 6 commits into from
Oct 4, 2020

Conversation

lehni
Copy link
Collaborator

@lehni lehni commented Sep 30, 2020

I noticed that the current line-length wasn't counted correctly depending on the setting of attributeSeparator.
This PR fixes that and adds tests for the various settings.

@Shinigami92 Shinigami92 marked this pull request as draft September 30, 2020 06:28
@Shinigami92
Copy link
Member

I really like the tokenNeedsSeparator member function, maybe we should extract it as a exported function to src/utils/common.ts. Then we could also write tests specifically for it 🤔

For an in depth review, I need more (free-)time 🙂

@lehni
Copy link
Collaborator Author

lehni commented Sep 30, 2020

Thanks! The function relies on state on the PugPrinter class currently which is read from the options (this.neverUseAttributeSeparator and this.alwaysUseAttributeSeparator), so doing this would be a bit tricky! Also, I'm not sure we need to test the function in isolation, since we're already testing its effects through the many tests relating to attributeSeparator.

@Shinigami92 Shinigami92 added the type: enhancement Functionality that enhances existing features label Oct 2, 2020
@Shinigami92
Copy link
Member

Sorry for the merge conflicts, cannot resolve them on my side 🙁
That will change when you later create branches in this repo directly 🙂

@lehni
Copy link
Collaborator Author

lehni commented Oct 2, 2020

No worries! Will resolve later!

@lehni lehni force-pushed the fix/current-line-length-counting branch from 5fd5178 to 5d5c64a Compare October 2, 2020 21:49
@lehni
Copy link
Collaborator Author

lehni commented Oct 2, 2020

@Shinigami92 I've rebased the changes in this branch on top of master, resolving all the conflicts, and force-pushed a new version. I hope that's fine!

@Shinigami92
Copy link
Member

@lehni I hope I'm not to deep into "how I organize PRs", but is this feature merge ready?
If now more work is planned on your side, you should click the Ready for review Button and also the Re-request review Icon 🙂
(Applies to all PRs)

@lehni lehni requested a review from Shinigami92 October 3, 2020 09:50
@lehni lehni marked this pull request as ready for review October 3, 2020 21:52
@lehni lehni force-pushed the fix/current-line-length-counting branch from fb82843 to 1db2e1c Compare October 3, 2020 21:54
Add comments explaining the handling of  literal attributes and normal ones
@lehni lehni force-pushed the fix/current-line-length-counting branch from 1db2e1c to deff459 Compare October 3, 2020 21:57
@lehni
Copy link
Collaborator Author

lehni commented Oct 3, 2020

@Shinigami92 when looking into the handling of literal attributes, I found a new bug in the current implementation of the plugin that happens as soon as you have both an id and class attribute on an element. I already have a fix for this in a follow up PR, but it will require #121 to be merged first. And I don't want to include it here, to not complicate this PR further.

@Shinigami92 Shinigami92 merged commit 1be2bdd into prettier:master Oct 4, 2020
@lehni lehni deleted the fix/current-line-length-counting branch July 22, 2023 13:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement Functionality that enhances existing features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants