Skip to content

Conversation

@ninio
Copy link

@ninio ninio commented Nov 19, 2020

The default value is false to keep the current behaviour as-is
Added both JS and Py implementations
Added unit tests for the space_in_paren option

Description

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

Fixes #1856

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) - It's already documented, but was not applied for CSS beautification.
  • README.md documents new feature/option(s) - It's already documented, but was not applied for CSS beautification.

The default value is false to keep the current behaviour as-is
Added both JS and Py implementations
Added unit tests for the space_in_paren option
Copy link
Member

@bitwiseman bitwiseman left a comment

Choose a reason for hiding this comment

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

Thanks for contributing!

Additional testing needed.

You might also try using a matrix config for the test. That way you can can supply a few inputs and generate the expected outputs like in the example below.

https://github.com/beautify-web/js-beautify/blob/3f1fff2b0ab58833806e9487de9b89945a371e46/test/data/css/tests.js#L1670-L1714

fragment: true,
input: [
'a {',
'width: min(100%,100vw);',
Copy link
Member

Choose a reason for hiding this comment

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

Please add another test where the input has multiple spaces in inside the paren, example: 'width: min(. 100%. , 100vw. );',. In the case the extra spaces should be removed.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the feedback! Will add them in the coming week.

@bitwiseman
Copy link
Member

@ninio Do you have time to update this?

Base automatically changed from master to main January 11, 2021 21:55
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.

Use the space_in_paren option in CSS files too.

2 participants