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 prettier to xo config and switch to two-space indentation #611

Closed
wants to merge 8 commits into from

Conversation

Kreeg
Copy link
Member

@Kreeg Kreeg commented Mar 19, 2022

@XhmikosR for me, conversations in commits are quite inconvenient. Let's communicate here.
Have you seen this changes? Do you agree/disagree with them?
Sorry in advance for this PR.

@XhmikosR
Copy link
Member

So, here we have 2 choices:

  1. try to stay close to the xo style we are already using
  2. go with prettier's defaults and adapt xo

In the first case, not sure what the gain is, though. Just being able to format other files except for js/ts etc?

I don't mind going with the second way, as long as we agree on a common set of options.

@Kreeg
Copy link
Member Author

Kreeg commented Mar 19, 2022

@XhmikosR let's go with second variant. Then step by step, file by file we will include prettier to all code base.
What line length do you prefer? I'm OK with 120. We may decrease it to 80

@Kreeg Kreeg changed the title WIP: Prettier adding prettier Mar 20, 2022
@Kreeg Kreeg marked this pull request as ready for review March 20, 2022 21:40
@Kreeg
Copy link
Member Author

Kreeg commented Mar 22, 2022

@XhmikosR should we land it?

And by the way, I think we should change trailingComma to es5 to add commas after each field in object and all elements in array. This is better for code review since it will cause the one-line change instead of two lines (comma + line break + new field / new element )

@XhmikosR
Copy link
Member

Please hold on to this. I haven't had a chance to look at it yet.

@XhmikosR XhmikosR marked this pull request as draft March 23, 2022 05:48
@Kreeg Kreeg changed the title adding prettier Added prettier to XO config Jun 1, 2022
@Kreeg Kreeg force-pushed the prettier branch 2 times, most recently from f5d6cca to 29545ce Compare September 4, 2022 12:40
@XhmikosR XhmikosR changed the title Added prettier to XO config Add prettier to xo config Apr 13, 2023
bin/svg-sprite.js Dismissed Show dismissed Hide dismissed
@XhmikosR XhmikosR changed the title Add prettier to xo config Add prettier to xo config and switch to two-space indentation Apr 13, 2023
@@ -3,17 +3,18 @@
'use strict';

/**
* svg-sprite is a Node.js module for creating SVG sprites
* Svg-sprite is a Node.js module for creating SVG sprites
Copy link
Member

Choose a reason for hiding this comment

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

Is there any way we don't touch the license headers?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, https://github.com/hosseinmd/prettier-plugin-jsdoc/#Options
jsdocCapitalizeDescription: false

@XhmikosR
Copy link
Member

XhmikosR commented Apr 13, 2023

So far I see the following:

  1. I don't like prettier using single quotes in yaml, but I can live with it. If there's a way to use double quotes for yaml/yml files only feel free to push
  2. I don't like the license header changes; they feel unnatural :/
  3. I don't like the new operator-linebreak option, but I guess I can live with it
  4. space-before-function-paren: can't we enforce this to never like we had?
  5. Indentation in jsdoc seems wrong now. For example:
    /**
     * Checks if value is a callable function.
     *
     * @param   {any}     value The value to check.
     *
     * @returns {boolean}       Returns true if value is correctly classified, else
     *   false.
     */
  6. I definitely don't like prettier's HTML formatting so I have disabled it (feel free to disable it in a better way if there's one)
  7. prettier chokes on our templates
  8. format check is not enforced on CI; we should probably add it there too if we go with this approach
  9. I switched to two-space indentation because it feels more natural, but it's hard to see the diff because it's huge

Overall, I don't mind proceeding with this as long as we sort the above :)

@Kreeg
Copy link
Member Author

Kreeg commented Apr 14, 2023

@XhmikosR this is a big list. I think in this case we should not use prettier. Otherwise we would struggle to fight against it. Prettier is a zero-config like solution, if there are so much factors with it's integration, then we can say it just does not fit here

@XhmikosR
Copy link
Member

XhmikosR commented May 8, 2023

Closing for now. I landed the 2-space indentation in #812

@XhmikosR XhmikosR closed this May 8, 2023
@XhmikosR XhmikosR deleted the prettier branch May 8, 2023 14:44
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.

None yet

2 participants