Skip to content

Instantly share code, notes, and snippets.

@JoshuaKGoldberg
Last active July 6, 2023 15:16
Show Gist options
  • Star 2 You must be signed in to star a gist
  • Fork 0 You must be signed in to fork a gist
  • Save JoshuaKGoldberg/cb7630833acf7421b6d93b51a333612d to your computer and use it in GitHub Desktop.
Save JoshuaKGoldberg/cb7630833acf7421b6d93b51a333612d to your computer and use it in GitHub Desktop.
DefinitelyFormatted

👉 This has since been posted to DefinitelyTyped's GitHub Discussions: DefinitelyTyped/DefinitelyTyped#65993 👈

Context

👋 Hi all! I've been working with the TypeScript team on overhauling DefinitelyTyped's usage of formatting and linting tools. Alongside the effort to finally migrate from TSLint to ESLint (https://typescript-eslint.io/linting/troubleshooting/tslint -> microsoft/DefinitelyTyped-tools#648), we'd like to align with how much of the industry & many linter maintainers recommend using a formatter vs. a linter (myself included).

This is a surfacing of a plan we've formulated that we think will get DefinitelyTyped onto using a proper formatter for formatting. I'd very much like to see the community's reaction: does this seem reasonable to you? What edge cases are we missing? Will this offend your sensibilities as a programmer?

DefinitelyTyped's Formatting Context

DefinitelyTyped today only utilizes a few ESLint rules for code formatting. It does not use Prettier, dprint, or other dedicated formatting tools.

We'd like to move DefinitelyTyped onto one of those tools for the following benefits:

  • Consistency: so contributors don't have to adjust to different formatting preferences per types package.
  • Dev time: formatters save contributor time from being spent on manually touching up files.
  • Diff sizes: so developers stop accidentally (or intentionally) reformatting entire files in the course of making an unrelated change.
  • Easier TSLint to ESLint migration: removing usage of formatting rules reduces the amount of logic we'll need to port from TSLint to ESLint

Please do not reply to this with formatting opinions. We know using a formatter instead of a linter for formatting can be a contentious choice. Bikeshedding on nuances of whitespace preferences is not useful.

The Plan

  1. Determine the formatter choice and formatting configuration
  2. Create and merge a DefinitelyTyped-tools pull request that removes all formatting rules
  3. Simultaneously to (2), create and merge a DefinitelyTyped pull request that:
    • Runs the formatter on all files in DefinitelyTyped
    • Adds checking in formatting fixes to the merge bot
    • Adds a GitHub Action that makes a new commit to format all files if needed after merges to master
  4. Create and merge a DefinitelyTyped pull request that adds a .git-blame-ignore-revs file to ignore the large formatting commit

Formatter Choice: dprint

The two most common standalone formatters in the JavaScript/TypeScript ecosystem today are:

  • dprint: much faster, but less popular
  • Prettier: more popular, but slower

We plan on using dprint for formatting for its superior performance.

A quick comparison of running both tools and pprettier on DefinitelyTyped shows a 10 second vs. minutes-scale difference:

npx dprint fmt  141.52s user 9.29s system 1481% cpu 10.177 total
npx pprettier --write './types/**/*.{ts,mts,cts}'  8.97s user 3.18s system 21% cpu 56.725 total
npx prettier -w ./types  361.28s user 35.60s system 126% cpu 5:13.36 total

dprint is also able to use a cacheable file cache, which drops times down to under a second. We may look into using it in CI as a followup performance optimization.

Additionally, running dprint on as large a repository as DefinitelyTyped will provide it and SWC with helpful community dogfooding (e.g. swc-project/swc#7187, dprint/dprint-plugin-typescript#533). Note that dprint is essentially deno fmt, so we're not worried about its ability to handle all of DefinitelyTyped.

Formatting Configuration

DefinitelyTyped today already has a .prettierrc.json Prettier configuration file:

{
  "$schema": "http://json.schemastore.org/prettierrc",
  "arrowParens": "avoid",
  "singleQuote": true,
  "trailingComma": "all",
  "tabWidth": 4,
  "printWidth": 120
}

Our plan is to use the closest equivalent dprint TypeScript configuration. This proposal does not include work to change those settings. No set of settings is perfect or will satisfy all users.

FAQs

Explainer: When Formatting Runs

Pull request CI builds will not be blocked on formatting, so as to not inconvenience contributors who don't run a formatter. However, two parts of the plan ensure the master branch stays fully formatted:

  • Most PRs are merged by the DT bot: which can apply apply formatting on each PR as part of its existing merge process.
  • For manual merges done by maintainers: the action that runs on merges to master will capture any formatting mishaps

As a follow-up, we may want to create a general-purpose PR bot to make it easier for contributors to apply formatting to a PR on their behalf. This is out of scope for the current proposal.

Can Files Opt Out of Formatting?

Yes, but considering the advantages mentioned earlier in this proposal, we strongly advise against doing so. Individual files that are auto-generated can use configuration file excludes and/or // dprint-ignore-file file ignore comments to disable the formatter.

Is Formatting Performance Important?

Most users would not notice a difference in performance between dprint and Prettier, as most files aren't large enough (thousands of lines long) to cause a significant difference. However, the CI runs on the master branch should ideally be as fast as possible to minimize the chance of users branching off a master commit with incorrectly unformatted code.

Tabs or Spaces?

One potential side effect of switching to a dedicated formatter is that individual projects that prefer using tabs for accessibility may find it harder to do so in the project. If that describes you, please post here and let us know your use case and why you need to use tabs.

Projects that want to still use tabs can always disable the formatter. Again, please let us know - if this would inconvenience you we can try to brainstorm a workaround.

Today, only 19 types directories use tabs - as determined by finding all types/ directories that have any file with a number of \n\ts greater than 1% of its total \n count. Therefore, this proposal is not changing how the vast majority (>99.9%) of types packages use spaces.

Script run to find types packages that use tabs
import fs from 'node:fs/promises';
import path from 'node:path';

async function checkDirectoryPathForTabs(directoryPath) {
    const directoryItems = await readdirSafe(directoryPath);

    for (const directoryItem of directoryItems ?? []) {
        const directoryItemPath = path.join(directoryPath, directoryItem);
        if (!directoryItemPath.includes('.ts')) {
            continue;
        }

        const contents = await readFileSafe(directoryItemPath);
        if (!contents) {
            continue;
        }

        const newlineTabsCount = contents.match(/\n\t/g)?.length ?? 0;
        if (!newlineTabsCount) {
            continue;
        }

        const newlineCount = contents.match(/\n/g)?.length ?? 0;

        if (newlineTabsCount > newlineCount / 100) {
            console.log(directoryPath)
            return;
        }
    }
}

async function readdirSafe(dirPath) {
    try {
        return await fs.readdir(dirPath);
    } catch {
        return undefined;
    }
}

async function readFileSafe(filePath) {
    try {
        return (await fs.readFile(filePath)).toString();
    } catch {
        return undefined;
    }
}

for (const directory of await fs.readdir('./types')) {
    const directoryPath = path.join('./types', directory);

    await checkDirectoryPathForTabs(directoryPath)
}
Package names logged by that script
types/anime-quotes-api
types/arcgis-js-api
types/bootbox
types/ceddl__ceddl-polyfill
types/classificator
types/dns-packet
types/do-not-zip
types/ej.web.all
types/gulp-help
types/gulp-inject
types/js2coffee
types/json-query
types/qbittorrent-api-v2
types/react-leaflet-markercluster
types/reefjs
types/steelseries
types/titanium
types/uuid4
types/vscode

What's the Difference Between a Formatter and a Linter?

A formatter quickly (re-)formats code without adjusting any code logic. A linter runs a set of discrete rules that each flag -and optionally suggest fixes- for a specific kind of code defect.

See https://github.com/readme/guides/formatters-linters-compilers for more context.

Thanks

This proposal came out of a group thread with discussion from:

Appreciation in particular to Jake Bailey and Nathan Shively-Sanders for advising from the TypeScript team, and David Sherret from dprint.

@sandersn
Copy link

sandersn commented Jul 5, 2023

Couple of editing points; I don't have any technical additions:

  • "much of the industry & many linter maintainers recommend": you're a prominent, long-time linter maintainer, so I'd find a way to put the second half of that in first-person since surely your recommendations are prominent in that set.
  • my github username is github.com/sandersn (predates my hyphenated name by half a decade)

@JoshuaKGoldberg
Copy link
Author

Adjusted for those, thanks!

I'm always hesitant to phrase myself as a reputable source. Partially out of shyness, and partially because I know there are quite a few linter-area folks more tenured than I who have different or less emphatic opinions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment