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

feat!: switch to prettier for formatting #259

Merged
merged 7 commits into from
Feb 6, 2019
Merged

Conversation

ofrobots
Copy link
Contributor

@ofrobots ofrobots commented Feb 5, 2019

NOTE: this targets the next branch.

BREAKING CHANGE: Switch to prettier as the code formatter.

@@ -0,0 +1,9 @@
module.exports = {
bracketSpacing: false,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know the point here isn't to paint the bikeshed, but I personally like bracket spacing ducks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you look at the delta, this is ported from the .prettierrc that you added.

Is there rationalization for the value you used?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I had just copied over what's used in all of the other libraries 🤷‍♂️ I doubt those choices were really scrutinized. I don't have a strong opinion here, just a preference for defaults when nobody otherwise cares. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated the config to a minimal set (IMO). PTAL.

prettier.config.js Outdated Show resolved Hide resolved
src/format.ts Outdated Show resolved Hide resolved
@JustinBeckwith
Copy link
Collaborator

Not sure if on purpose or a typo - why the ! after feat?

@ofrobots
Copy link
Contributor Author

ofrobots commented Feb 5, 2019

@JustinBeckwith
Copy link
Collaborator

TIL! That's an awesome idea folks. I have no forking clue which of my commits today are breaking changes.

BREAKING CHANGE: Switch to prettier as the code formatter.

Fixes: google#156
@ofrobots
Copy link
Contributor Author

ofrobots commented Feb 5, 2019

@google/google-node-team any opinions on the default prettier config?

@codecov-io
Copy link

codecov-io commented Feb 5, 2019

Codecov Report

Merging #259 into next will decrease coverage by 0.11%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##             next     #259      +/-   ##
==========================================
- Coverage   98.29%   98.17%   -0.12%     
==========================================
  Files          12       13       +1     
  Lines         823      769      -54     
  Branches       69       61       -8     
==========================================
- Hits          809      755      -54     
  Misses         14       14
Impacted Files Coverage Δ
src/format.ts 100% <100%> (ø) ⬆️
src/clean.ts 100% <100%> (ø) ⬆️
test/test-lint.ts 100% <100%> (ø) ⬆️
src/init.ts 88.99% <100%> (ø) ⬆️
prettier.config.js 100% <100%> (ø)
test/test-clean.ts 100% <100%> (ø) ⬆️
src/util.ts 100% <100%> (ø) ⬆️
src/lint.ts 100% <100%> (ø) ⬆️
test/test-init.ts 100% <100%> (ø) ⬆️
test/fixtures.ts 100% <100%> (ø) ⬆️
... and 5 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 8604378...2bbe559. Read the comment docs.

@ofrobots
Copy link
Contributor Author

ofrobots commented Feb 5, 2019

NOTE to reviewers: the third commit is a result of reformatting. It would be easier to review just the first two commits independently.

Copy link
Collaborator

@JustinBeckwith JustinBeckwith left a comment

Choose a reason for hiding this comment

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

This is flippin awesome.

Copy link
Contributor

@kjin kjin left a comment

Choose a reason for hiding this comment

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

LGTM, and I'm good with this config

Copy link
Contributor

@kjin kjin left a comment

Choose a reason for hiding this comment

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

LGTM w/ nits

src/format.ts Outdated Show resolved Hide resolved
src/format.ts Outdated Show resolved Hide resolved
@ofrobots
Copy link
Contributor Author

ofrobots commented Feb 6, 2019

PTAL.

test.serial('format should leave the kitty unharmed', t => {
const KITTY = `
/\\**/\\
( o_o )_)
Copy link
Contributor

Choose a reason for hiding this comment

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

🐱

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants