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

Create script for checks (prettier, lint, flow) in CI #93

Merged
merged 3 commits into from Oct 9, 2017

Conversation

jxom
Copy link
Contributor

@jxom jxom commented Oct 9, 2017

This intends to fix #67.

  • Added a check-all:verbose script, which is similar to check-all, however lists errors out on prettier if the files are different from the prettier formatting.
  • Added a prettier:diff script, which checks for a difference between the prettified files, and source files.

Yay or nay?

@jxom jxom changed the title Create script for checks in ci Create script for checks (prettier, lint, flow) in CI Oct 9, 2017
@reactjs-bot
Copy link

reactjs-bot commented Oct 9, 2017

Deploy preview ready!

Built with commit 5a96b1d

https://deploy-preview-93--reactjs.netlify.com

@bvaughn
Copy link
Contributor

bvaughn commented Oct 9, 2017

Some thoughts! 😄

  • I would rename check-all:verbose to something like ci-check just to make its purpose clearer 😄
  • lint isn't failing when there are errors like unused variables- but it should. I've probably just totally misconfigured it. Maybe we should copy over something similar to the React project's lint config? The goal is to get a useful set of lint checks that don't conflict with Prettier's config.

@jxom
Copy link
Contributor Author

jxom commented Oct 9, 2017

@bvaughn - yeah was a bit unsure about what to name it!

I was wondering why the linter didn’t pick up stuff like that too! Thought there may have been some reason why you configured it that way. 😆

Will check it out later on!

@bvaughn
Copy link
Contributor

bvaughn commented Oct 9, 2017

Nope. I was trying for a minimal linter config and didn't test it thoroughly enough at the time.

Copy link
Contributor

@bvaughn bvaughn 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 great! Catches a lot of small lint errors that have slipped through too 😄

Looks like we may have to do something special for certain Flow types, eg:

/Users/bvaughn/Documents/git/reactjs.org/src/theme.js
  48:13  error  '$Keys' is not defined  no-undef

But we can iterate on things like this and not hook the new Yarn task into CI until they've all been fixed.

@bvaughn bvaughn merged commit 8f50d04 into reactjs:master Oct 9, 2017
jhonmike pushed a commit to jhonmike/reactjs.org that referenced this pull request Jul 1, 2020
BetterZxx pushed a commit to BetterZxx/react.dev that referenced this pull request Mar 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CI should run Lint, Flow, and Prettier checks for every PR
4 participants