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 support for loading custom formatter from package #6228

Merged
merged 9 commits into from Jul 29, 2022
Merged

Add support for loading custom formatter from package #6228

merged 9 commits into from Jul 29, 2022

Conversation

meriouma
Copy link
Contributor

@meriouma meriouma commented Jul 27, 2022

Which issue, if any, is this issue related to?

Closes #6100

Is there anything in the PR that needs further explanation?

No, it's self-explanatory.

lib/resolveCustomFormatter.js Outdated Show resolved Hide resolved
lib/resolveCustomFormatter.js Outdated Show resolved Hide resolved
lib/resolveCustomFormatter.js Outdated Show resolved Hide resolved
lib/resolveCustomFormatter.js Outdated Show resolved Hide resolved
@JounQin
Copy link
Member

JounQin commented Jul 28, 2022

@meriouma Don't do squash and force push locally, keep the commits' history, that's fine. We'll merge with squash.

@JounQin JounQin requested a review from ybiquitous July 28, 2022 02:09
Copy link
Member

@ybiquitous ybiquitous left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change looks good to me. I've left a suggestion for testing.

lib/cli.js Show resolved Hide resolved
Copy link
Member

@ybiquitous ybiquitous left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggest] Sorry, I missed one point. Could you make single exporting consistent with other modules?

lib/__tests__/resolveCustomFormatter.test.js Outdated Show resolved Hide resolved
lib/cli.js Outdated Show resolved Hide resolved
lib/resolveCustomFormatter.js Outdated Show resolved Hide resolved
Co-authored-by: Masafumi Koba <473530+ybiquitous@users.noreply.github.com>
Copy link
Member

@ybiquitous ybiquitous left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggest] I think it's good to improve the CLI help and the doc about the --custom-formatter option. It will support a package name, not only a relative/absolute file path.

Could you update also the following files?

stylelint/lib/cli.js

Lines 178 to 180 in 743030d

--custom-formatter
Path to a JS file exporting a custom formatting function.

The `formatter` Node.js API option can also accept a function, whereas the `--custom-formatter` CLI flag accepts a path to a JS file exporting one. The function in both cases must fit the signature described in the [Developer Guide](../../developer-guide/formatters.md).

@ybiquitous ybiquitous changed the title Add support for loading custom formatter from package when using PnP Add support for loading custom formatter from package Jul 28, 2022
@JounQin
Copy link
Member

JounQin commented Jul 28, 2022

@meriouma Are you still working on this?

@meriouma
Copy link
Contributor Author

@JounQin Yes, I will apply the changes. I stopped pushing changes yesterday so we were not two persons pushing in the branch at the same time.

@JounQin
Copy link
Member

JounQin commented Jul 28, 2022

@JounQin Yes, I will apply the changes. I stopped pushing changes yesterday so we were not two persons pushing in the branch at the same time.

You'll only need to pull the latest codes and then continue the work. 🍺

@ybiquitous
Copy link
Member

@meriouma Thank you for addressing the requested changes! The PR looks almost good to me.

I got an idea to test a custom formatter dependency in the repository: to install a private internal dependency. For example:

package.json:

{
  "devDependencies": {
    "stylelint-test-custom-formatter": "file:packages/stylelint-test-custom-formatter"
  }
}

packages/:

$ tree packages
packages/
└── stylelint-test-custom-formatter/
    ├── index.js
    └── package.json

1 directory, 2 files

Run a test:

$ bin/stylelint.js --config=scripts/visual-config.json --custom-formatter=stylelint-test-custom-formatter scripts/*.css
Custom formatter reports 1 warning(s).%

What do you think about this idea?

@JounQin
Copy link
Member

JounQin commented Jul 28, 2022

@ybiquitous jest.mock can also be used in cli.test.js to cover this?

@ybiquitous
Copy link
Member

@JounQin I thought about how to test the dependency behavior without mocking. But mocking dependency resolution in cli.test.js may be enough and straightforward.

@JounQin
Copy link
Member

JounQin commented Jul 28, 2022

@JounQin I thought about how to test the dependency behavior without mocking. But mocking dependency resolution in cli.test.js may be enough and straightforward.

Yeah, a mock module like https://github.com/stylelint/stylelint/blob/main/lib/__tests__/__mocks__/get-stdin.js is fine IMO.

(Besides, https://github.com/stylelint/stylelint/blob/main/lib/__tests__/__mocks__/get-stdin.js seems can be deleted safely in favor of #6182, see #6233

Copy link
Member

@ybiquitous ybiquitous left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! LGTM 👍🏼

@JounQin JounQin merged commit e271601 into stylelint:main Jul 29, 2022
@JounQin
Copy link
Member

JounQin commented Jul 29, 2022

@meriouma Thanks for your contribution!

@ybiquitous
Copy link
Member

Changelog entry added:

  • Added: support for loading custom formatter from package (#6228).

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

Successfully merging this pull request may close these issues.

Add support for PnP as linked customFormatters
3 participants