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 minimum attributes option for attribute wrap #1818

Merged
merged 7 commits into from
May 23, 2023

Conversation

ang-zeyu
Copy link
Contributor

@ang-zeyu ang-zeyu commented Jul 18, 2020

Description

  • Source branch in your fork has meaningful name (not master)

Fixes Issue: #1758

Usage:

Added a wrap_attributes_min_attrs / -M option (default 2).

It only affects force/force-aligned/force-expand-multiline currently.
Does it make sense for the other non preserve options to be affected as well?

if preferred, I could take a shot at #1404 before this. But since this is an extension to the spec proposed there, it should work out even if it's dealt with later

Implementation:

I simply moved the lookahead for force-expand-multiline to handle_tag_open, and changed the relevant parts accordingly. This might affect performance a little for force/force-aligned options.

Alternatively, it could be done at the tokenizer level with an even smaller performance hit (since the read_attribute handler has easy access to open_token). I'm not sure if there's something against extending token for separate beautifiers though

Before Merge Checklist

These items can be completed after PR is created.

(Check any items that are not applicable (NA) for this PR)

  • JavaScript implementation
  • Python implementation (NA if HTML beautifier)
  • Added Tests to data file(s)
  • Added command-line option(s) (NA if
  • README.md documents new feature/option(s)

@bitwiseman
Copy link
Member

@ang-zeyu
Sorry, you'll need to resolve your change against the new master branch.

@ang-zeyu
Copy link
Contributor Author

Done, thanks!

What do you think of the scope of the usage? Should it affect auto and aligned-multiple?

@bitwiseman
Copy link
Member

@ang-zeyu
I'm sorry, I have not had time to evaluate this change. I'm going to publish a new version without merging this. This is not comment on the quality of this change. I simply haven't had bandwidth to consider this change properly.

@ang-zeyu
Copy link
Contributor Author

@ang-zeyu
I'm sorry, I have not had time to evaluate this change. I'm going to publish a new version without merging this. This is not comment on the quality of this change. I simply haven't had bandwidth to consider this change properly.

Hi @bitwiseman, noted, no worries! We aren't under any schedule here 🙂

@quantizor
Copy link

Can this be made to work with CSS as well? Would love to set this to -1 or an arbitrarily high number so I can get all CSS property rules on a single line

@bitwiseman
Copy link
Member

@probablyup
The CSS beautifier is far behind the other beautifiers in development. The others tokenize the input and then beautify it. CSS is processed freeform, meaning it does benefit from most featured implemented for the other beautifiers.

Base automatically changed from master to main January 11, 2021 21:55
@ang-zeyu
Copy link
Contributor Author

I've changed the behaviour here a little since last (d8bcf29) for the force-expand-multiline option - min_attrs=1 is now valid for it, e.g.

<input type="text" />

// now possible
<input
    type="text"
/>

backward compatibility is still kept since the default min_attrs=2.

@bumbummen99
Copy link

I would love to see this feature in js-beautify as well as VS Code HTML formatter.

@Ahrengot
Copy link

Yes, same here. I'm using https://github.com/shufo/blade-formatter and that repo also has multiple open issues for this same feature. Would be really awesome to have this.

@bitwiseman bitwiseman self-requested a review April 22, 2023 22:59
@savaonepunch
Copy link

Would love this!

@bitwiseman bitwiseman merged commit a849063 into beautifier:main May 23, 2023
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.

6 participants