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

Change default value of trailingComma to "all" #11479

Merged
merged 11 commits into from
Aug 1, 2022
Merged

Conversation

fisker
Copy link
Member

@fisker fisker commented Sep 9, 2021

Description

Similar to #6963, set trailingComma default value to "all", closes #11465

Checklist

  • I’ve added tests to confirm my change works.
  • (If changing the API or CLI) I’ve documented the changes I’ve made (in the docs/ directory).
  • (If the change is user-facing) I’ve added my changes to changelog_unreleased/*/XXXX.md file following changelog_unreleased/TEMPLATE.md.
  • I’ve read the contributing guidelines.

Try the playground for this PR

@Zamiell
Copy link

Zamiell commented Jan 8, 2022

this also closes #9369
(#11465 is a duplicate of #9369)

are there any updates on this PR?

@Zamiell
Copy link

Zamiell commented Jul 18, 2022

@fisker Can you respond as to whether you are going to work on this PR? Support for Internet Explorer, the only browser lacking an update for modern trailing comma support, has ended on June 15, 2022.

@fisker
Copy link
Member Author

fisker commented Jul 18, 2022

@Zamiell

You can still set trailingComma to "none" or "es5", we are only going to change the default value.

We'll update snapshots and merge before v3 release, it's a simple PR.

@sosukesuzuki sosukesuzuki mentioned this pull request Jul 23, 2022
23 tasks
fisker added 2 commits July 23, 2022 21:35
# Conflicts:
#	tests/format/angular/angular/jsfmt.spec.js
#	tests/format/flow/function-parentheses/jsfmt.spec.js
#	tests/format/flow/generic/jsfmt.spec.js
#	tests/format/flow/object-order/jsfmt.spec.js
#	tests/format/flow/parameter-with-type/jsfmt.spec.js
#	tests/format/graphql/trailing-comma/jsfmt.spec.js
#	tests/format/js/arrow-call/jsfmt.spec.js
#	tests/format/js/destructuring-ignore/jsfmt.spec.js
#	tests/format/js/object-property-ignore/jsfmt.spec.js
#	tests/format/js/strings/jsfmt.spec.js
#	tests/format/js/trailing-comma/jsfmt.spec.js
#	tests/format/json/json/jsfmt.spec.js
#	tests/format/json/with-comment/jsfmt.spec.js
#	tests/format/lwc/lwc/jsfmt.spec.js
#	tests/format/misc/json-unknown-extension/jsfmt.spec.js
#	tests/format/scss/map/jsfmt.spec.js
#	tests/format/scss/scss/jsfmt.spec.js
#	tests/format/scss/trailing-comma/jsfmt.spec.js
#	tests/format/typescript/angular-component-examples/jsfmt.spec.js
#	tests/format/typescript/interface2/jsfmt.spec.js
#	tests/format/typescript/tuple/jsfmt.spec.js
#	tests/format/vue/custom_block/jsfmt.spec.js
#	tests/format/vue/vue/jsfmt.spec.js
@github-actions

This comment was marked as outdated.

@fisker
Copy link
Member Author

fisker commented Jul 23, 2022

For reviewers , use this link instead of click the Files changed tab.

@fisker fisker marked this pull request as ready for review July 23, 2022 16:10
@fisker fisker requested a review from sosukesuzuki July 23, 2022 16:12
@fisker fisker changed the title Set trailingComma default value to "all" Change default value of trailingComma to "all" Jul 23, 2022
# Conflicts:
#	tests/format/js/strings/__snapshots__/jsfmt.spec.js.snap
#	tests/format/js/strings/jsfmt.spec.js
# Conflicts:
#	tests/format/js/strings/jsfmt.spec.js
github-merge-queue bot pushed a commit to microsoft/qsharp that referenced this pull request Oct 27, 2023
Updated all direct npm dependencies to `@latest`.

Observed changes:
- Prettier: now adds trailing commas to all lists.
prettier/prettier#11479
- Katas: `marked` now omits `id` attributes for header tags (confirmed
benign) markedjs/marked#2890
- Other trivial differences in JS emit through `esbuild`.

I was really hoping this would be a trivial process, but sadly it
wasn't. For posterity, here's what the process ended up being:

```js
const { execSync } = require("node:child_process");

function run(command) {
  console.log(`running command ${command}`);
  return execSync(command);
}

const deps = JSON.parse(run(`npm ls --package-lock-only --json`));

// Do these in this order first, otherwise @typescript-eslint/eslint-plugin
// gets really upset about the peer dependency missing
run("npm install @typescript-eslint/parser@latest");
run("npm install @typescript-eslint/eslint-plugin@latest");

// Update all direct deps to @latest
for (const depName of Object.keys(deps.dependencies)) {
  if (!depName.startsWith("qsharp")) {
    run(`npm install ${depName}@latest`);
  }
}

// Update indirect deps in package-lock.json
run("npm update");

// This fixes the workspace names in package-lock.json,
// which for some reason get erased above
run("npm install");

```

Ran `./build.py` to confirm the repo still built, discovered formatting
changes.

To reformat with new Prettier defaults:

```bash
npm run prettier:fix
```

Since we pulled a lot of JS tooling updates, I ran a quick script to
save the built artifacts so I could diff the JS output:

```bash
#!/bin/bash

set -e

# clean repo
git clean -fdx
rm -rf baseline-compare/
mkdir baseline-compare

# build
./build.py --wasm --npm --vscode --no-check --no-test

# npm
mkdir -p baseline-compare/npm
pushd npm
npm pack --pack-destination .
popd
tar -xzf npm/qsharp-lang-0.0.0.tgz -C baseline-compare/npm

# vsix
npm install -g @vscode/vsce
pushd vscode
vsce package --pre-release
popd
unzip vscode/qsharp-lang-vscode-dev-0.0.0.vsix -d baseline-compare/vscode

# commit baseline
cp package-lock.json baseline-compare/
git add baseline-compare/
git commit -m "Baseline for $(git rev-parse HEAD)"
```

Ran this script first on `main`, then with the updated `package.json` &
`package-lock.json`. Used `git` to diff the `baseline-compare` changes.
Confirmed they looked harmless.
medikoo pushed a commit to medikoo/prettier-elastic that referenced this pull request Jan 17, 2024
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.

4 participants