From 55312289116d4577532f77e247ed2b1bae0bddcb Mon Sep 17 00:00:00 2001 From: Jesse Rosenberger Date: Sat, 31 Aug 2019 21:00:10 +0300 Subject: [PATCH] Remove destructive code formatting tooling. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This change should not stop any developers from using auto code formatting tools (e.g. Prettier, which was in place prior to this commit). Developers who would like to keep using such tools merely need to highlight their changes and choose the "Format Selection" (or similar) option in their editor. If they choose to use Prettier, no configuration file is necessary; this project is accepting of the default Prettier settings, so long as they aren't used to reformat entire files/code-bases. For more details, please read this incredibly long commit message. I hope it'll help highlight another side to automated, enforced, opinionated code-formatting tools. <3 Code formatting is, as I think this long commit message will continue to point out, a subjective preference. Code formatting tools, like Prettier, have tried to end formatting disagreements by picking a single set of "unwavering" rules. It's hard to win at this, but as long as I'm involved with this project, I will not be nit-picking anyone's choice of code-style so long as it is not intentionally trying to be unnecessarily unorthodox. Also, even if it is, in my opinion, appearing to be unnecessarily unorthodox I pledge to try and understand and be empathetic to the reasoning for it. I would encourage others to evaluate, discuss, and react similarly! At a high-level, my preference is: Try to match surrounding style, leave comments when necessary, try to think about how to best represent your change itself and consider the future of the code — at least briefly. That said, the formatting of code isn't always always best determined by an opinionated tool. On more than one occasion, Prettier has made decisions for this project which have resulted in decreased code clarity in our changesets (i.e. pull-requests; PRs). For example, even removing a single character from a variable can result in a changeset with anywhere from ~2 to several-hundred of lines of unnecessary changes. As a very small example of this, consider [4bd1f901db][1], a change to a misspelled variable, which resulted in the Prettification seen in [d6b78d0cb93940][2]. Now amplify that change to a large function where the function parameters changed by plus or minute two characters and the hanging indent unnecessarily changes so that it can fit more things on the previous line. When I first ran into the spelling example above, I was bothered and again tempted to remove Prettier. I knew that I had previous examples of this amplified change being problematic, but couldn't quickly dig up an example to make a case. Low and behold, it only took five days before another example came up!: See the relatively simple change in [c413141ae775][3], which stats out to: packages/apollo-server-koa/src/__tests__/ApolloServer.test.ts | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) Now see what Prettier does to this brief change in [8c6fc24cfbd16][4]: packages/apollo-server-koa/src/__tests__/ApolloServer.test.ts | 1124 +++++++++++++++++++------------------- 1 file changed, 562 insertions(+), 562 deletions(-) With Prettier (or most opinionated automatic code formatting tools), the entire function must now be reformatted when the clarity of the function was not at all in jeopardy! Not only is this unnecessarily changing code, but it also buries "blame" for these changes across an extra ~500 lines of code through repeated rounds of similar re-linting. In my opinion, those additional lines being changed aren't at all useful for the digestibility of a project. When I open a PR, for the success of the code, I prefer that the changes not be clouded with artifacts that make it more difficult for the reviewer to understand. Conversely, when I'm reviewing a PR, I prefer to avoid the same. Unrelated formatting changes — automatic or not — are the most frequent contributors to this pain, in my experience. As lesser, but still related points: It's not at all uncommon for "opinionated" tools to change their opinions. On more than one occasion, I've updated a dependency in this repository only to have to re-format swaths of code: - https://github.com/apollographql/apollo-server/pull/846 - https://github.com/apollographql/apollo-server/pull/2203 - https://github.com/apollographql/apollo-server/pull/2572 Plus, on other occasions, I've had to wade through over-complicated merge conflicts between branches when otherwise capable (Git) merge strategies were sandbagged by formatting differences. All this said, I want to emphasize that I really encourage anyone to use tools that facilitate the automatic formatting of their code. I just ask that they can do it with a limited scope that merely touches the code they're changing (e.g. "Format Selection"). If there is no way to avoid it and it's absolutely necessary to re-format hundreds of lines of code, that's okay! There are certainly cases where very large reformatting is warranted, but it seems better if it's the exception rather than the norm. I hope that there's an understanding that there are times that tools that work for one developer/project/concept, it won't work for another. As a regular contributor to this project, I personally find that Prettier frustrates me more often than it helps me (long-term and short-term considerations both being had). Conversely, I know that Prettier greatly simplifies the workflow for many! I love that! But while someone might really enjoy `editor.formatOnSave` re-formatting their code, imagine my own surprise when it does exactly that. Dialogue and conversation is very welcome here. I certainly don't want to push anyone away from contributing, but I need to also be realistic about my own frustrations. I'm excited for tooling (which apparently doesn't exist yet!) that can help improve this for those on either side of the Prettier picket fence, but until that time comes, I think Prettier will better left to the concern of the developers who wish to employ it. [1]: https://github.com/apollographql/apollo-server/commit/4bd1f901dbeceb69638703cfa724576666fc2e31 [2]: https://github.com/apollographql/apollo-server/commit/d6b78d0cb93940675b193627695b04d2ceb59ae7 [3]: https://github.com/apollographql/apollo-server/commit/c413141ae775ed0b1ef8b6d4c193ac563f5b6c49 [4]: https://github.com/apollographql/apollo-server/commit/8c6fc24cfbd16fde0acfe1a72aa4b5fc0cfb5d66 --- .circleci/config.yml | 12 ----- .vscode/settings.json | 6 +++ CONTRIBUTING.md | 2 +- package-lock.json | 116 +----------------------------------------- package.json | 5 -- 5 files changed, 9 insertions(+), 132 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 9d77a5e1940..8a6051da0af 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -73,16 +73,6 @@ jobs: docker: [{ image: 'circleci/node:12' }] <<: *common_test_steps - # Other tests, unrelated to typical code tests. - Linting: - docker: [{ image: 'circleci/node:10' }] - steps: - # (speed) Intentionally omitted, unnecessary, run_install_desired_npm. - - checkout - # (speed) --ignore-scripts to skip unnecessary Lerna build during linting. - - run: npm install --ignore-scripts - - run: npm run lint - # XXX We used to use this filter to only run a "Docs" job on docs branches. # Now we use it to disable all jobs. It's unclear if there's a simpler way # to do this! @@ -104,5 +94,3 @@ workflows: <<: *ignore_doc_branches - Node.js 12: <<: *ignore_doc_branches - - Linting: - <<: *ignore_doc_branches diff --git a/.vscode/settings.json b/.vscode/settings.json index 2802354f200..4085c295aaa 100644 --- a/.vscode/settings.json +++ b/.vscode/settings.json @@ -1,6 +1,12 @@ // Place your settings in this file to overwrite default and user settings. { "editor.tabSize": 2, + // You're welcome to use Prettier on code hunks that are directly related to + // your changes but we ask that, rather than using "Format Document" or + // `editor.formatOnSave`, you instead highlight your changes and choose + // "Format Selection" (Windows: Ctrl-K Ctrl-F; Mac: Cmd-K Cmd-F) to avoid + // destructive formatting changes to entire files. Thanks for understanding! + "editor.formatOnSave": false, "editor.rulers": [80], "editor.wordWrapColumn": 80, "files.trimTrailingWhitespace": true, diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 392bd3c1abd..c48c2153032 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -79,7 +79,7 @@ It’s important that every piece of code in Apollo packages is reviewed by at l 3. **Testing.** Do the tests ensure this code won’t break when other stuff changes around it? When it does break, will the tests added help us identify which part of the library has the problem? Did we cover an appropriate set of edge cases? Look at the test coverage report if there is one. Are all significant code paths in the new code exercised at least once? 4. **No unnecessary or unrelated changes.** PRs shouldn’t come with random formatting changes, especially in unrelated parts of the code. If there is some refactoring that needs to be done, it should be in a separate PR from a bug fix or feature, if possible. 5. **Code has appropriate comments.** Code should be commented, or written in a clear “self-documenting” way. -6. **Idiomatic use of the language.** In TypeScript, make sure the typings are specific and correct. In ES2015, make sure to use imports rather than require and const instead of var, etc. Ideally a linter enforces a lot of this, but use your common sense and follow the style of the surrounding code. +6. **Idiomatic use of the language.** In TypeScript, make sure the typings are specific and correct. In ES2015, make sure to use imports rather than require and const instead of var, etc. Use your common sense and follow the style of the surrounding code. ## New contributors diff --git a/package-lock.json b/package-lock.json index d497cfd29ef..c79754e2256 100644 --- a/package-lock.json +++ b/package-lock.json @@ -4923,12 +4923,6 @@ "integrity": "sha512-MQcXEUbCKtEo7bhqEs6560Hyd4XaovZlO/k9V3hjVUF/zwW7KBVdSK4gIt/bzwS9MbR5qob+F5jusZsb0YQK2A==", "dev": true }, - "builtin-modules": { - "version": "1.1.1", - "resolved": "https://registry.npmjs.org/builtin-modules/-/builtin-modules-1.1.1.tgz", - "integrity": "sha1-Jw8HbFpywC9bZaR9+Uxf46J4iS8=", - "dev": true - }, "builtins": { "version": "1.0.3", "resolved": "https://registry.npmjs.org/builtins/-/builtins-1.0.3.tgz", @@ -5320,7 +5314,8 @@ "version": "2.17.1", "resolved": "https://registry.npmjs.org/commander/-/commander-2.17.1.tgz", "integrity": "sha512-wPMUt6FnH2yzG95SA6mzjQOEKUU3aLaDEmzs1ti+1E9h+CsrZghRlqEM/EJ4KscsQVG8uNN4uVreUeT8+drlgg==", - "dev": true + "dev": true, + "optional": true }, "compare-func": { "version": "1.3.2", @@ -6086,12 +6081,6 @@ "streamsearch": "0.1.2" } }, - "diff": { - "version": "3.5.0", - "resolved": "https://registry.npmjs.org/diff/-/diff-3.5.0.tgz", - "integrity": "sha512-A46qtFgd+g7pDZinpnwiRJtxbC1hpgf0uzP3iG89scHk0AUC7A1TGxf5OiiOUv/JMZR8GOt8hL900hV0bOy5xA==", - "dev": true - }, "diff-sequences": { "version": "24.9.0", "resolved": "https://registry.npmjs.org/diff-sequences/-/diff-sequences-24.9.0.tgz", @@ -13534,71 +13523,6 @@ "integrity": "sha1-IZMqVJ9eUv/ZqCf1cOBL5iqX2lQ=", "dev": true }, - "prettier": { - "version": "1.18.2", - "resolved": "https://registry.npmjs.org/prettier/-/prettier-1.18.2.tgz", - "integrity": "sha512-OeHeMc0JhFE9idD4ZdtNibzY0+TPHSpSSb9h8FqtP+YnoZZ1sl8Vc9b1sasjfymH3SonAF4QcA2+mzHPhMvIiw==", - "dev": true - }, - "prettier-check": { - "version": "2.0.0", - "resolved": "https://registry.npmjs.org/prettier-check/-/prettier-check-2.0.0.tgz", - "integrity": "sha512-HZG53XQTJ9Cyi5hi1VFVVFxdlhITJybpZAch3ib9KqI05VUxV+F5Hip0GhSWRItrlDzVyqjSoDQ9KqIn7AHYyw==", - "dev": true, - "requires": { - "execa": "^0.6.0" - }, - "dependencies": { - "cross-spawn": { - "version": "5.1.0", - "resolved": "https://registry.npmjs.org/cross-spawn/-/cross-spawn-5.1.0.tgz", - "integrity": "sha1-6L0O/uWPz/b4+UUQoKVUu/ojVEk=", - "dev": true, - "requires": { - "lru-cache": "^4.0.1", - "shebang-command": "^1.2.0", - "which": "^1.2.9" - } - }, - "execa": { - "version": "0.6.3", - "resolved": "https://registry.npmjs.org/execa/-/execa-0.6.3.tgz", - "integrity": "sha1-V7aaWU8IF1nGnlNw8NF7nLEWWP4=", - "dev": true, - "requires": { - "cross-spawn": "^5.0.1", - "get-stream": "^3.0.0", - "is-stream": "^1.1.0", - "npm-run-path": "^2.0.0", - "p-finally": "^1.0.0", - "signal-exit": "^3.0.0", - "strip-eof": "^1.0.0" - } - }, - "get-stream": { - "version": "3.0.0", - "resolved": "https://registry.npmjs.org/get-stream/-/get-stream-3.0.0.tgz", - "integrity": "sha1-jpQ9E1jcN1VQVOy+LtsFqhdO3hQ=", - "dev": true - }, - "lru-cache": { - "version": "4.1.5", - "resolved": "https://registry.npmjs.org/lru-cache/-/lru-cache-4.1.5.tgz", - "integrity": "sha512-sWZlbEP2OsHNkXrMl5GYk/jKk70MBng6UU4YI/qGDYbgf6YbP4EvmqISbXCoJiRKs+1bSpFHVgQxvJ17F2li5g==", - "dev": true, - "requires": { - "pseudomap": "^1.0.2", - "yallist": "^2.1.2" - } - }, - "yallist": { - "version": "2.1.2", - "resolved": "https://registry.npmjs.org/yallist/-/yallist-2.1.2.tgz", - "integrity": "sha1-HBH5IY8HYImkfdUS+TxmmaaoHVI=", - "dev": true - } - } - }, "pretty-format": { "version": "24.7.0", "resolved": "https://registry.npmjs.org/pretty-format/-/pretty-format-24.7.0.tgz", @@ -13723,12 +13647,6 @@ "ipaddr.js": "1.8.0" } }, - "pseudomap": { - "version": "1.0.2", - "resolved": "https://registry.npmjs.org/pseudomap/-/pseudomap-1.0.2.tgz", - "integrity": "sha1-8FKijacOYYkX7wqKw0wa5aaChrM=", - "dev": true - }, "psl": { "version": "1.1.31", "resolved": "https://registry.npmjs.org/psl/-/psl-1.1.31.tgz", @@ -15291,36 +15209,6 @@ "resolved": "https://registry.npmjs.org/tslib/-/tslib-1.9.3.tgz", "integrity": "sha512-4krF8scpejhaOgqzBEcGM7yDIEfi0/8+8zDRZhNZZ2kjmHJ4hv3zCbQWxoJGz1iw5U0Jl0nma13xzHXcncMavQ==" }, - "tslint": { - "version": "5.19.0", - "resolved": "https://registry.npmjs.org/tslint/-/tslint-5.19.0.tgz", - "integrity": "sha512-1LwwtBxfRJZnUvoS9c0uj8XQtAnyhWr9KlNvDIdB+oXyT+VpsOAaEhEgKi1HrZ8rq0ki/AAnbGSv4KM6/AfVZw==", - "dev": true, - "requires": { - "@babel/code-frame": "^7.0.0", - "builtin-modules": "^1.1.1", - "chalk": "^2.3.0", - "commander": "^2.12.1", - "diff": "^3.2.0", - "glob": "^7.1.1", - "js-yaml": "^3.13.1", - "minimatch": "^3.0.4", - "mkdirp": "^0.5.1", - "resolve": "^1.3.2", - "semver": "^5.3.0", - "tslib": "^1.8.0", - "tsutils": "^2.29.0" - } - }, - "tsutils": { - "version": "2.29.0", - "resolved": "https://registry.npmjs.org/tsutils/-/tsutils-2.29.0.tgz", - "integrity": "sha512-g5JVHCIJwzfISaXpXE1qvNalca5Jwob6FjI4AoPlqMusJ6ftFE7IkkFoMhVLRgK+4Kx3gkzb8UZK5t5yTTvEmA==", - "dev": true, - "requires": { - "tslib": "^1.8.1" - } - }, "tunnel-agent": { "version": "0.6.0", "resolved": "https://registry.npmjs.org/tunnel-agent/-/tunnel-agent-0.6.0.tgz", diff --git a/package.json b/package.json index 79369cf6a7d..372d81b1517 100644 --- a/package.json +++ b/package.json @@ -12,8 +12,6 @@ "release:server": "npm run release -- --force-publish=apollo-server,apollo-server-core,apollo-server-azure-functions,apollo-server-cloud-functions,apollo-server-cloudflare,apollo-server-express,apollo-server-fastify,apollo-server-hapi,apollo-server-koa,apollo-server-lambda,apollo-server-micro,apollo-server-testing,apollo-server-integration-testsuite,apollo-server-env,apollo-tracing,apollo-server-types,graphql-extensions,apollo-cache-control,apollo-datasource,apollo-engine-reporting,apollo-server-errors,apollo-server-plugin-base", "release:federation": "npm run release -- --force-publish=@apollo/federation,@apollo/gateway", "postinstall": "lerna run prepare && npm run compile", - "lint": "prettier-check '**/*.{js,ts}'", - "lint-fix": "prettier '**/*.{js,ts}' --write", "test": "jest --verbose", "test:clean": "jest --clearCache", "test:watch": "jest --verbose --watchAll", @@ -123,8 +121,6 @@ "mock-req": "0.2.0", "nock": "10.0.6", "node-fetch": "2.3.0", - "prettier": "1.18.2", - "prettier-check": "2.0.0", "qs-middleware": "1.0.3", "request": "2.88.0", "request-promise": "4.2.4", @@ -132,7 +128,6 @@ "supertest": "4.0.2", "test-listen": "1.1.0", "ts-jest": "24.0.2", - "tslint": "5.19.0", "typescript": "3.5.3", "ws": "6.2.1" },