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

fix: Bumped eslint, prettier-eslint to latest with updates #97

Merged
merged 2 commits into from
Aug 30, 2017

Conversation

jmenglis
Copy link
Contributor

@jmenglis jmenglis commented Aug 30, 2017

Bumped eslint to 4.5.0 and prettier-eslint to 6.4.2. Updated eslint-config-kentcdodds to 12.4.2.
Added some changes to package.json to correct new formatting of error rules form eslint. Imported
Config and Lint from eslint/lib to pass into the new ConfigFile.load(file, context)

fix: update to eslint 4.0 #94

What:

Bumped eslint to 4.5.0 and prettier-eslint to 6.4.2. Updated eslint-config-kentcdodds to 12.4.2. Added some changes to package.json to correct new formatting of error rules form eslint. Imported Config and Lint from eslint/lib to pass into the new ConfigFile.load(file, context)

Why:

Want to get prettier-eslint-cli to the latest version of eslint and prettier-eslint

How:

Updated packages. Added some additional logic to load method for loading config. Updated eslint rules to include new eslint standards for semi and object curly braces.

Bumped eslint to 4.5.0 and prettier-eslint to 6.4.2. Updated eslint-config-kentcdodds to 12.4.2.
Added some changes to package.json to correct new formatting of error rules form eslint.  Imported
Config and Lint from eslint/lib to pass into the new ConfigFile.load(file, context)

BREAKING CHANGE: Updating to eslint latest is a breaking change as we need to pass context into the
.load function

fix: update to eslint 4.0 prettier#94
@codecov-io
Copy link

codecov-io commented Aug 30, 2017

Codecov Report

Merging #97 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #97   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           3      3           
  Lines         107    108    +1     
=====================================
+ Hits          107    108    +1
Impacted Files Coverage Δ
src/format-files.js 100% <100%> (ø) ⬆️

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 8ffe4ad...f2e91ac. Read the comment docs.

Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

Thank you so much for working on this! Just a few questions 😄

],
"object-curly-spacing": [
"error",
"never"
Copy link
Member

Choose a reason for hiding this comment

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

Are these changes necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The tests do not pass without these changes. I took a look at the config repo and did not see the cases for this. Therefore when the npm start validate ran the files were handled according to the rules but two failed to occur. The removal of a ; and the spacing around an object import { something } from 'something vs import {something} from 'something'. The test required tha latter to pass.

if (eslintConfigPath) {
prettierESLintOptions.eslintConfig = ConfigFile.load(eslintConfigPath)
prettierESLintOptions.eslintConfig =
ConfigFile.load(eslintConfigPath, configContext)
Copy link
Member

Choose a reason for hiding this comment

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

There's nothing that's changing about the external API right? So this is technically not a breaking change I think.

@@ -49,9 +51,10 @@ function formatFilesFromArgv({
prettierLast,
prettierOptions,
}

const configContext = new Config({}, new Linter())
Copy link
Member

Choose a reason for hiding this comment

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

Also, it looks like configContext is only used within the if statement below, could we move this declaration to within that if statement?

Updates after PR Review to move const location

BREAKING CHANGE: None

prettier#94
Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

Thanks a bunch!

@kentcdodds kentcdodds merged commit 0238687 into prettier:master Aug 30, 2017
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.

None yet

3 participants