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

Turn on prettier (except repo root) #1167

Merged
merged 17 commits into from
Aug 8, 2018
Merged

Turn on prettier (except repo root) #1167

merged 17 commits into from
Aug 8, 2018

Conversation

paulmelnikow
Copy link
Member

@paulmelnikow paulmelnikow commented Oct 12, 2017

Follow-up to #1123.

This is going to conflict with open PRs. Before merging it, I want to get through a few open PRs which are making substantive changes in lib/:

The open PRs for service badges will conflict too, though the conflicts there will be smaller and so not as difficult to solve.

@paulmelnikow paulmelnikow added the developer-experience Dev tooling, test framework, and CI label Oct 12, 2017
@paulmelnikow
Copy link
Member Author

I proposed at #948 (comment) that we turn off semicolons for lib/ and service-tests/ before merging this.

Copy link
Member

@Daniel15 Daniel15 left a comment

Choose a reason for hiding this comment

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

Seems good to me. I don't really have any comments to add, since this is all automated.

lib/badge.js Outdated
' Generated untemplated SVG:\n' +
'---\n' + untemplatedSvg + '---\n');
console.error(
'Template ' +
Copy link
Member

Choose a reason for hiding this comment

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

Looks like a great use case for a format string.

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 a prettier option for prefer template string?

@platan
Copy link
Member

platan commented Oct 18, 2017

Do we want to add info about usage of prettier in this project in documentation, e.g. in CONTRIBUTING.md? And maybe one example command how to format code using prettier (or other tool). I'm using prettier for the first time and I found such commands prettier --write "{lib,service-tests}/**/*.js" and npm run lint -- --fix (which runs eslint --fix). We can configure pre-commit hooks as well.

@FezVrasta
Copy link
Contributor

pre-commit hook would make much more sense than explaining commands to the contributors, the process should be transparent

@paulmelnikow
Copy link
Member Author

paulmelnikow commented Jul 31, 2018

I'm not sure we'll be able to merge all of these in a timely way. How about we just go ahead and do this, and deal with the rebasing + conflicts? I worry if we wait until everything is clear we could be waiting a long time…

@RedSparr0w
Copy link
Member

Yeah, that sounds fine to me 👍

@paulmelnikow
Copy link
Member Author

@chris48s Does that work for you, too? Most of the outstanding rebasing work would likely fall on the three of us, so if we're cool with it, I'll revive this, update it, add some doc, and then merge it shortly after.

@chris48s
Copy link
Member

chris48s commented Aug 1, 2018

Now seems like a good time to get this rebase and in. #1808 is in now. Happy to do the work to rebase #1740 as I'd like to update it once we hammer out #1743. Lets get this done before we start anything else big 👍

@shields-ci
Copy link

shields-ci commented Aug 2, 2018

Messages
📖

✨ Thanks for your contribution to Shields, @paulmelnikow!

📖

Thanks for contributing to our documentation. We ❤️ our documentarians!

Generated by 🚫 dangerJS

@paulmelnikow paulmelnikow changed the title Turn on prettier for service-tests/ and lib/ Turn on prettier (except server.js) Aug 2, 2018
@paulmelnikow paulmelnikow changed the title Turn on prettier (except server.js) Turn on prettier (except repo root) Aug 2, 2018
@paulmelnikow
Copy link
Member Author

I spot-checked these and they look way cleaner, especially without the semis. The tester code looks especially pretty! ✨

Copy link
Member

@RedSparr0w RedSparr0w left a comment

Choose a reason for hiding this comment

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

Nice work, Changes are looking good.

@Daniel15
Copy link
Member

Daniel15 commented Aug 6, 2018

Nice work! I miss the semicolons, but consistency is good. I'm happy either way, as long as it's consistent across everything :)

@paulmelnikow
Copy link
Member Author

To make it easier to read the config changes, I reverted the formatting changes, which I'll merge separately. This PR will fail prettier-check in CI.

@paulmelnikow paulmelnikow merged commit ab051b3 into badges:master Aug 8, 2018
@shields-deployment
Copy link

This pull request was merged to master branch. Now this change is waiting for deployment.
Deploys usually happen every few weeks. After deployment changes are copied to gh-pages branch.

This badge displays deployment status:

@paulmelnikow paulmelnikow deleted the prettier branch August 8, 2018 21:49
@paulmelnikow paulmelnikow mentioned this pull request Aug 8, 2018
paulmelnikow added a commit that referenced this pull request Aug 8, 2018
Merging this separately so the commit with the tooling change is readable. This is a follow-on to #1167 which turned prettier on.
@paulmelnikow
Copy link
Member Author

I've posted guidance on resolving merge conflicts here: #1866 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
developer-experience Dev tooling, test framework, and CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants