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

chore: use prettier #401

Merged
merged 1 commit into from
Jun 30, 2022
Merged

chore: use prettier #401

merged 1 commit into from
Jun 30, 2022

Conversation

westeezy
Copy link
Contributor

No description provided.

@westeezy westeezy requested review from gregjopa and wsbrunson April 20, 2022 19:50
@codecov-commenter
Copy link

codecov-commenter commented Apr 20, 2022

Codecov Report

Merging #401 (ee5595e) into main (2fe2858) will increase coverage by 0.09%.
The diff coverage is 95.56%.

@@            Coverage Diff             @@
##             main     #401      +/-   ##
==========================================
+ Coverage   95.53%   95.63%   +0.09%     
==========================================
  Files          18       18              
  Lines        1186     1213      +27     
==========================================
+ Hits         1133     1160      +27     
  Misses         53       53              
Impacted Files Coverage Δ
src/lib/global.js 69.23% <69.23%> (ø)
src/drivers/vue3.js 87.50% <87.50%> (ø)
src/lib/serialize.js 91.11% <90.90%> (ø)
src/component/templates/component.js 92.30% <92.30%> (ø)
src/drivers/angular2.js 93.54% <93.33%> (ø)
src/component/props.js 93.93% <93.75%> (ø)
src/component/component.js 95.42% <95.32%> (+0.02%) ⬆️
src/parent/parent.js 95.60% <95.60%> (+0.22%) ⬆️
src/drivers/angular.js 95.65% <95.65%> (ø)
src/lib/window.js 96.22% <96.07%> (+0.07%) ⬆️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2fe2858...ee5595e. Read the comment docs.

@bluepnume
Copy link
Collaborator

Can we start with a prettier config that approximately matches the current lint style settings? (e.g. single quotes for strings)

@westeezy
Copy link
Contributor Author

The goal overall to move style rules out of linting and use a base prettier config. So the first question I have: is there a concern you have with moving to prettier's default config? I know it will be a large diff but hoping back commits is quite easy in most editors. Also if we use prettier we will have to forgo at least some style rules even if we attempted to match such as the spacing of object keys, so if the goal is to keep the same style rules we have now then I would say no to prettier and keep them in eslint.

@bluepnume
Copy link
Collaborator

Sure; I mean at least with the single quote thing:

  • It's a minor tweak in the config that saves on a huge number of unnecessary changes
  • Single quotes are easier to type. Can type them one handed, with no shift key. 😂

@westeezy
Copy link
Contributor Author

Sure; I mean at least with the single quote thing:

  • It's a minor tweak in the config that saves on a huge number of unnecessary changes
  • Single quotes are easier to type. Can type them one handed, with no shift key. 😂

Ah perfect I just want to be sure we are on the same page in terms of moving to prettier is agreed upon. So in the spirit of changes to prettier I think it'll likely be single quotes, 4 units on indent, and 120 line length. I'd have to check if anything else in the docs quickly. I'll also have to double check if we were tabs or spaces quickly since I just set my editors to auto-format to our rules in grumbler-scripts and didn't pay attention.

@bluepnume
Copy link
Collaborator

Ya for sure, totally happy with prettier, just prefer to do things incrementally when possible. So yeah would be good to stick with single quotes / 4 spaces / etc. for now.

@gregjopa gregjopa marked this pull request as ready for review June 28, 2022 14:32
@gregjopa gregjopa requested a review from a team as a code owner June 28, 2022 14:32
Copy link
Contributor

@gregjopa gregjopa left a comment

Choose a reason for hiding this comment

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

Looks good! I'm a big fan of using the defaults with the prettier config.

@westeezy
Copy link
Contributor Author

After chatting with the larger team a bit more we have concluded that we would like to pursue the base prettier config and that its probably easiest to just rip the bandaid all at once so we are going to merge this with base config. Of course that means git blaming will typically require you to go back an extra commit but eventually that'd have to be the case to get to the base config for prettier anyways.

@westeezy westeezy merged commit 3974c52 into main Jun 30, 2022
@westeezy westeezy deleted the prettier branch June 30, 2022 14:45
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.

5 participants