chore: prettier reset trailingComma: all #972

acao · 2019-10-05T20:32:15Z

in the eslint upgrade and typescript conversion kerfuffle we accidentally reverted to es5 commas briefly. re-adding all those es2017 compatible commas!

codecov · 2019-10-05T20:48:20Z

Codecov Report

Merging #972 into master will increase coverage by 0.01%.
The diff coverage is 37.03%.

@@            Coverage Diff             @@
##           master     #972      +/-   ##
==========================================
+ Coverage   42.43%   42.45%   +0.01%     
==========================================
  Files          64       64              
  Lines        2922     2923       +1     
  Branches      633      633              
==========================================
+ Hits         1240     1241       +1     
  Misses       1411     1411              
  Partials      271      271

Impacted Files	Coverage Δ
...ervice-interface/src/getAutocompleteSuggestions.ts	`69.39% <ø> (ø)`	⬆️
...hql-language-service-server/src/GraphQLWatchman.js	`0% <ø> (ø)`	⬆️
...s/graphiql/src/components/DocExplorer/SearchBox.js	`58.33% <ø> (ø)`	⬆️
packages/graphiql/src/utility/QueryStore.js	`26.92% <ø> (ø)`	⬆️
packages/graphiql/src/components/ToolbarSelect.js	`6.45% <ø> (ø)`	⬆️
...ages/graphql-language-service-server/src/Logger.js	`5.55% <ø> (ø)`	⬆️
packages/graphiql/src/utility/find.js	`33.33% <ø> (ø)`	⬆️
...kages/graphql-language-service-parser/src/Rules.ts	`0% <ø> (ø)`	⬆️
...ge-service-interface/src/GraphQLLanguageService.ts	`44.94% <ø> (ø)`	⬆️
packages/graphql-language-service/src/cli.js	`0% <ø> (ø)`	⬆️
... and 19 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 2da818b...64f1796. Read the comment docs.

acao · 2019-10-06T21:23:49Z

this is failing because it changes code coverage :/

Neitsch

Why is there a new file packages/examples/graphiql-cdn/graphiql.css?

Could you also point me to the config where you enabled the trailing comma? I can't find it amongst all those changes :/

acao · 2019-10-07T13:18:17Z

@Neitsch this needs rebased but that new file will already exist as of the new master
The change is to .prettierrc. In this PR, we change from es5 trailingCommas (which is why some commas were just recently stripped, because we had recently applied this rule) back to all trailingCommas (since es2017 supports trailing commas in functions).

acao · 2019-10-07T13:20:26Z

as you can see though, adding these extra commas somehow decreases coverage significantly

acao · 2019-10-07T13:31:40Z

@Neitsch again, everything passes but codecov's test coverage comparison :/

benjie · 2019-10-07T14:41:08Z

.eslintignore

 /packages/graphiql/test/vendor
+
+# Funky codemirror-graphql build pattern
+/packages/codemirror-graphql/**


You have to be very careful with this pattern; if this line matches a directory then files in that directory will not be re-included by the ! rule. ESLint uses the same algorithm as gitignore; which states:

An optional prefix "!" which negates the pattern; any matching file excluded by a previous pattern will become included again. It is not possible to re-include a file if a parent directory of that file is excluded. Git doesn’t list excluded directories for performance reasons, so any patterns on contained files have no effect, no matter where they are defined.

To fix you have to match JUST the files explicitly:

Suggested change

/packages/codemirror-graphql/**

/packages/codemirror-graphql/**/*.ts

/packages/codemirror-graphql/**/*.tsx

/packages/codemirror-graphql/**/*.js

/packages/codemirror-graphql/**/*.jsx

Maybe we should note this more strongly here:

graphiql/.eslintignore

Line 7 in 2da818b

# First: ignore FILES (not folders!)

ok, the above suggestion works except it needs to ignore everything but src though. the whole issue with codemirror-graphql is that it builds to the root for easy resolutions, and the new eslintignore file doesnt ignore the built files anymore. the subpackage .gitignore honors this and we need the .eslinignore to do the same

Yes; that's why I left the !.../src/** line below :) It's only the actual /** line I changed to make more explicit to only match files and not folders. I believe this will achieve the outcome you desire.

benjie

Couple notes 👍

benjie · 2019-10-07T14:42:52Z

.prettierignore

+
+# breaks prettier graphql parser
+packages/graphql-language-service-server/src/__tests__/__queries__/test.graphql
+packages/codemirror-graphql/src/__tests__/schema-kitchen-sink.graphql


Maybe we should ignore all the graphql files in __tests__; some of them might test whitespace?

good point... that reminds me of at least one test "fix" i had to make that was suspicious in this PR...

benjie · 2019-10-07T14:48:03Z

resources/pretty.js

@@ -14,7 +14,7 @@ const INVERSE = '\x1b[7m';
 const RESET = '\x1b[0m';
 const YELLOW = '\x1b[33m';

-const options = ['--trailing-comma=es5'];
+const options = ['--config ../../.prettierrc'];


These options should be separate (for spawn), and we should pass a fixed path so that it doesn't matter where the script is called from.

Suggested change

const options = ['--config ../../.prettierrc'];

const options = ['--config', `${__dirname}/../.prettierrc`];

ah yes, absolute path is much better.

really, the script is so simple now, we could just invoke it directly

acao · 2019-10-08T14:20:39Z

@benjie once we are done with this PR (it looks like we will have to add a fair amount of test coverage to make this one pass) and some of the other big ones, lets sit down with @IvanGoncharov, you and myself and do one last pass at auditing our eslint/prettier setup

benjie · 2019-10-10T13:15:29Z

Sounds like a plan. Fair warning: I am not a fan of YAML.

Neitsch · 2019-10-14T12:01:01Z

Thanks for fixing the trailing comma stuff! Let's ship it 👍
Then we can address @benjie improvement suggestions

acao · 2019-10-14T12:28:08Z

@Neitsch @benjie i think we should scrap this PR personally. We already have es5 commas, so all object literals have trailing commas, etc. The commit you saw was where we enforced es5 commas, stripping out the extra es2018 commas downstream. This all option just adds them to function parameters. We are gonna need to add a bunch of test coverage to the LSP parser or server to get this to pass. So it could take a while, and its a huge cross cutting Pr so it will break other active ones. I know this is just the cost of making changes, but just for cosmetic ones? If we must...

benjie · 2019-10-15T10:13:12Z

I wouldn't worry about increasing code coverage when the changes are just formatting. It's okay to ignore that one status check :)

benjie

@acao I'd like to see this PR done as two separate commits:

Commit 1: changes the configuration of ESLint/prettier/etc
Commit 2: autofix changes ONLY - no manual changes.

This will make it a lot easier to review.

I believe we can completely ignore codecov for this PR.

acao · 2019-10-15T15:48:23Z

I agree on the approach, but I think we should just close it. we dont even need this config change. we already have es5 commas

…

On Tue, Oct 15, 2019 at 6:17 AM Benjie Gillam ***@***.***> wrote: ***@***.**** requested changes on this pull request. @acao <https://github.com/acao> I'd like to see this PR done as two separate commits: - Commit 1: changes the configuration of ESLint/prettier/etc - Commit 2: autofix changes ONLY - no manual changes. This will make it a lot easier to review. I believe we can completely ignore codecov for this PR. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#972?email_source=notifications&email_token=AAKOFF6TO2RK3JNTFDXBUFTQOWKDZA5CNFSM4I5ZHHK2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCH6RMZI#pullrequestreview-301799013>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAKOFF7TOQ4OVMO5R4F5TNTQOWKDZANCNFSM4I5ZHHKQ> .

benjie · 2019-10-16T11:16:44Z

It's certainly not a big priority. Let's close it and come back to it later.

acao requested review from Neitsch and benjie October 5, 2019 20:33

acao force-pushed the reset-prettier branch from 04fa9bb to 3f2b103 Compare October 5, 2019 20:45

Neitsch suggested changes Oct 7, 2019

View reviewed changes

acao force-pushed the reset-prettier branch from 3f2b103 to 3a27431 Compare October 7, 2019 13:25

chore: prettier reset trailingComma: all

64f1796

acao force-pushed the reset-prettier branch from 3a27431 to 64f1796 Compare October 7, 2019 13:35

benjie reviewed Oct 7, 2019

View reviewed changes

benjie requested changes Oct 7, 2019

View reviewed changes

benjie requested changes Oct 15, 2019

View reviewed changes

benjie closed this Oct 16, 2019

acao deleted the reset-prettier branch December 2, 2019 01:59

snyk-bot mentioned this pull request Mar 30, 2021

[Snyk] Security upgrade yargs from 7.1.1 to 10.0.0 saurabharch/graphiql#18

Open

acao mentioned this pull request Jun 18, 2023

remove initialTabs. Use defaultTabs instead #3207

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: prettier reset trailingComma: all #972

chore: prettier reset trailingComma: all #972

acao commented Oct 5, 2019

codecov bot commented Oct 5, 2019 •

edited

Loading

acao commented Oct 6, 2019

Neitsch left a comment

acao commented Oct 7, 2019 •

edited

Loading

acao commented Oct 7, 2019

acao commented Oct 7, 2019

benjie Oct 7, 2019 •

edited

Loading

acao Oct 8, 2019

benjie Oct 15, 2019

benjie left a comment

benjie Oct 7, 2019

acao Oct 8, 2019

benjie Oct 7, 2019

acao Oct 8, 2019

acao Oct 8, 2019

acao commented Oct 8, 2019 •

edited

Loading

benjie commented Oct 10, 2019

Neitsch commented Oct 14, 2019

acao commented Oct 14, 2019 •

edited

Loading

benjie commented Oct 15, 2019

benjie left a comment

acao commented Oct 15, 2019 via email

benjie commented Oct 16, 2019

-/packages/codemirror-graphql/**
+/packages/codemirror-graphql/**/*.ts
+/packages/codemirror-graphql/**/*.tsx
+/packages/codemirror-graphql/**/*.js
+/packages/codemirror-graphql/**/*.jsx

	const options = ['--config ../../.prettierrc'];
	const options = ['--config', `${__dirname}/../.prettierrc`];

chore: prettier reset trailingComma: all #972

chore: prettier reset trailingComma: all #972

Conversation

acao commented Oct 5, 2019

codecov bot commented Oct 5, 2019 • edited Loading

Codecov Report

acao commented Oct 6, 2019

Neitsch left a comment

Choose a reason for hiding this comment

acao commented Oct 7, 2019 • edited Loading

acao commented Oct 7, 2019

acao commented Oct 7, 2019

benjie Oct 7, 2019 • edited Loading

Choose a reason for hiding this comment

acao Oct 8, 2019

Choose a reason for hiding this comment

benjie Oct 15, 2019

Choose a reason for hiding this comment

benjie left a comment

Choose a reason for hiding this comment

benjie Oct 7, 2019

Choose a reason for hiding this comment

acao Oct 8, 2019

Choose a reason for hiding this comment

benjie Oct 7, 2019

Choose a reason for hiding this comment

acao Oct 8, 2019

Choose a reason for hiding this comment

acao Oct 8, 2019

Choose a reason for hiding this comment

acao commented Oct 8, 2019 • edited Loading

benjie commented Oct 10, 2019

Neitsch commented Oct 14, 2019

acao commented Oct 14, 2019 • edited Loading

benjie commented Oct 15, 2019

benjie left a comment

Choose a reason for hiding this comment

acao commented Oct 15, 2019 via email

benjie commented Oct 16, 2019

codecov bot commented Oct 5, 2019 •

edited

Loading

acao commented Oct 7, 2019 •

edited

Loading

benjie Oct 7, 2019 •

edited

Loading

acao commented Oct 8, 2019 •

edited

Loading

acao commented Oct 14, 2019 •

edited

Loading