Skip to content
This repository has been archived by the owner on May 4, 2022. It is now read-only.

style(js,json,less): use 'prettier' to handle code style #580

Closed

Conversation

ChristianMurphy
Copy link
Contributor

@ChristianMurphy ChristianMurphy commented Oct 15, 2017

Rather than throwing an error during precommit hook.
prettier will reformat all the code to meet the styleguide standards automatically.

Check out this talk for more information on the 'why' to use prettier.

part 1 of #535
part 2 is in #581


Contributor License Agreement adherence:

prettier reduces the barrier to entry, by automatically handling all js
and less code styling automatically.
Unlike ESLint every rule, in prettier is automatically fixable, with
special attention payed to how line wrapping is handled.
remove eslint disables intended for use with eslint-config-google
@ChristianMurphy ChristianMurphy changed the title Style: use 'prettier' to handle code style style(js,json,less): use 'prettier' to handle code style Oct 15, 2017
Copy link
Contributor

@davidmsibley davidmsibley left a comment

Choose a reason for hiding this comment

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

This looks like it's replacing single quotes to double quotes, breaking the google style guide?

@ChristianMurphy
Copy link
Contributor Author

@davidmsibley Currently I have prettier setup to use all of its defaults.
Those defaults are very similar, but subtly different than the Google Code style, we can adjust the settings to use single quotes.

@ChristianMurphy
Copy link
Contributor Author

@davidmsibley updated to use single quotes

@davidmsibley
Copy link
Contributor

I'm confused about a couple things in this. I gotta read it more, but I was referring to the single to double quote replacement that was happening in the js, not the less. Now the less is switched from double to single quotes.

Unrelated to whether or not we use prettier, I'm WAY more opposed to code being changed on commit than stopping you from committing. That sounds like many, many unintended problems waiting to happen.

@ChristianMurphy
Copy link
Contributor Author

😆 The codeclimate failure is occurring because the normalized code style allows codeclimate to detect nine places with code similarity that it wasn't able to before.

@vertein
Copy link
Contributor

vertein commented Oct 18, 2017

Does this automatically re-format to use Google's code style or does this use a separate style that we would have to match to Google's?

@apetro
Copy link
Contributor

apetro commented Oct 18, 2017

subtly different than the Google Code style

Okay, I hadn't understood that nuance. -1. Code is either in Google Style or it's not. Automatically different, even subtly, is not Google Style. I'd only be open to automatic code styling if it's automatically styling to the adopted code style. :)

This also may be a tool that it'd be best to adopt on a less-core module or three before adopting it here in uportal-application-framework. I'm not sure I share @davidmsibley 's prediction of many unintended problems, but I certainly see how a reasonable person would fear that. I think we can be way more open to experimental friction and risk in modules that we're less likely to need to release frequently for adoption by other teams, which is what this module is.

@ChristianMurphy
Copy link
Contributor Author

Code is either in Google Style or it's not. Automatically different, even subtly, is not Google Style.

Does this automatically re-format to use Google's code style or does this use a separate style that we would have to match to Google's?

The subtle differences are trailing commas and quotes.
Both can be configured to match Google Code Style. ✅

I'd only be open to automatic code styling if it's automatically styling to the adopted code style.

Or we could make the automated style the adopted style. 😉

That sounds like many, many unintended problems waiting to happen.

I understand the concern, in the past formatters could be flaky. 😨

Prettier and ESLint are based of ESTree an AST standard developed by Mozilla for accurately representing and safely manipulating code structure. 👍
Using AST for code manipulation resolves the invalid parsing and serialization errors common in regular expression based parsers (which did have major issues with producing invalid code). ✅

Furthermore, Prettier is in production use today on some major projects: React, Jest, Yarn, and others.
The software is pretty well battle tested. ⚔️

This also may be a tool that it'd be best to adopt on a less-core module or three before adopting it here in uportal-application-framework

🤷‍♂️ With the clean breakup of modules, it is arguably questionable if there is a "core" and what isn't core. Until recently I thought uPortal-home was "core".

I think we can be way more open to experimental friction and risk in modules that we're less likely to need to release frequently for adoption by other teams, which is what this module is.

Since all the modules are closely interrelated, what is module is sufficiently "low risk" in your mind?

@apetro
Copy link
Contributor

apetro commented Oct 18, 2017

  • uPortal-home is lower risk than uPortal-app-framework, in that only the latter has downstream projects depending upon it.
  • widget-creator is much lower risk, in that its audience is developers.

@ChristianMurphy
Copy link
Contributor Author

Closing this, til decision is made over in UW-Madison-DoIT/widget-creator#25

@ChristianMurphy ChristianMurphy deleted the style/use-prettier branch November 2, 2017 16:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants