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

Add postprocess config file option (useful for applying prettier formatting) #285

Merged
merged 14 commits into from Nov 27, 2022
Merged

Add postprocess config file option (useful for applying prettier formatting) #285

merged 14 commits into from Nov 27, 2022

Conversation

G-Rath
Copy link
Contributor

@G-Rath G-Rath commented Nov 25, 2022

This makes it easy to efficiently apply formatters like prettier after docs have been updated but before they're written to disk - this saves having to run such formatters across more files than necessary which on large codebases can be a decent time save.

Since it requires a function, you have to use a js-based config file i.e.:

const prettier = require('prettier');
const { prettier: prettierRC } = require('./package.json');

const config = {
  postprocess: content =>
    prettier.format(content, { ...prettierRC, parser: 'markdown' }),
};

module.exports = config;

The function is expected to return a promise to future-proof, since prettier is making all its public APIs async in v3, and it's all being done in an async function so there's not a strong downside.

This should be fully implemented (its working well locally with eslint-plugin-jest), but I've not had a chance to write any tests (I'm actually having some trouble with the cli tests not completing) so am opening as a draft to get an initial review and check that you're interested in this feature.

Also a possible extension to this could be to support a path to a file that exports a function when being used from the command line, though that might be overkill and should be landable in a followup PR anyway.

@bmish bmish added the enhancement New feature or request label Nov 25, 2022
@bmish
Copy link
Owner

bmish commented Nov 25, 2022

Awesome! This is much better / more flexible than the built-in prettier formatting I had implemented in a previous version (removed in #188).

Aside from tests, it will need to be mentioned in the README.md of course.

And do you think the single-word spelling postprocess is more common than postProcess?

Also as a heads up, I'll be releasing v1.0.0 very soon (days). This isn't a breaking change so it doesn't block anything.

@G-Rath
Copy link
Contributor Author

G-Rath commented Nov 25, 2022

do you think the single-word spelling postprocess is more common than postProcess?

I favored postprocess because that's what the matching config option is called in ESLint itself, and it seemed fitting to match.

@G-Rath
Copy link
Contributor Author

G-Rath commented Nov 26, 2022

@bmish I'd appreciate any pointers you might have on what tests you'd like and where to put them :)

@bmish
Copy link
Owner

bmish commented Nov 26, 2022

Since this is a new option, you can create option-postprocess-test.ts which passes this option with a trivial post-processing function (perhaps just adding a comment to the file), uses mockFs like most of the other tests, and then ensures the output file has the correct post-processing applied (comparing to a snapshot)

@bmish
Copy link
Owner

bmish commented Nov 26, 2022

For the README, we can add an example config file using this option to the prettier section. We can mention and link to that section from the Configuration options section.

lib/cli.ts Outdated Show resolved Hide resolved
@G-Rath
Copy link
Contributor Author

G-Rath commented Nov 26, 2022

hmm @bmish I'm having some trouble which might be related to #288, where my new test file just hangs indefinitely apparently due to mock-fs i.e.

import mockFs from 'mock-fs';

describe('my test', () => {
  it('is true', () => {
    expect(true).toBe(true);
  });
});

Results in:

eslint-doc-generator on  support-postprocess [!+?] is 📦 v0.26.1 via  v16.15.0 took 7s
❯ nrun test option-postprocess-test
> nrun.js test
> node --max-old-space-size=4096 --experimental-vm-modules node_modules/jest/bin/jest.js --colors --coverage option-postprocess-test
jest-haste-map: Haste module naming collision: eslint-plugin-test
  The following files share their name; please adjust your hasteImpl:
    * <rootDir>/test/fixtures/cjs/package.json
    * <rootDir>/test/fixtures/cjs-config-extends/package.json


 RUNS  test/lib/generate/option-postprocess-test.ts
^C

eslint-doc-generator on  support-postprocess [!+?] is 📦 v0.26.1 via  v16.15.0 took 11m35s

I was having this earlier with the CLI tests but they seemed to have come right - it feels like maybe something is trying to compile or load every single file on disk or in the project (including node_modules), though I can't rule that out easily due to the heavy use of enums which prevents isolatedModules from being used.

I'll try and dig into it a bit further, but would be good to have your eyes on it too.

@bmish
Copy link
Owner

bmish commented Nov 26, 2022

A few things:

  • Avoid using Node 14 locally to avoid Node 14 Windows CI tests failing sometimes #288
  • Make sure tsc compiles everything without errors--if there are any type issues, the tests will hang
  • Make sure to copy / start from one of the existing tests with mock-fs as there's some setup needed to get it to work correctly

@G-Rath
Copy link
Contributor Author

G-Rath commented Nov 26, 2022

if there are any type issues, the tests will hang

hmm right ok looks like this was the issue, thanks.

test/lib/cli-test.ts Outdated Show resolved Hide resolved
test/lib/cli-test.ts Outdated Show resolved Hide resolved
@G-Rath G-Rath marked this pull request as ready for review November 26, 2022 23:38
README.md Outdated Show resolved Hide resolved
@G-Rath G-Rath requested a review from bmish November 27, 2022 00:03
README.md Outdated Show resolved Hide resolved
Copy link
Owner

@bmish bmish 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! This is great.

@bmish bmish changed the title feat: add postprocess option for applying formatters like prettier Add postprocess config file option (useful for applying prettier formatting) Nov 27, 2022
@bmish bmish merged commit 150fdb9 into bmish:main Nov 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants