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

Configure severity with unified-lint-rule #67

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jablko
Copy link

@jablko jablko commented Mar 3, 2022

Initial checklist

  • I read the support docs
  • I read the contributing guide
  • I agree to follow the code of conduct
  • I searched issues and couldn’t find anything (or linked relevant results below)
  • If applicable, I’ve added docs and tests

Description of changes

What do you think about configuring the message severity in the standard way, using unified-lint-rule? e.g.

remark().use(remarkValidateLinks, [
  "error",
  { repository: "https://github.com/remarkjs/remark-validate-links.git" },
]);

I wrapped the remarkValidateLinks transformer in lintRule() and pass it the raw options, maybe with a severity.

@github-actions github-actions bot added the 👋 phase/new Post is being triaged automatically label Mar 3, 2022
@github-actions
Copy link

github-actions bot commented Mar 3, 2022

Hi! It seems some of the things asked in the template are missing? Please edit your post to fill out everything.

  • Initial checklist (todo)
  • Description of changes

You won’t get any more notifications from me, but I’ll keep on updating this comment, and remove it when done!

If you need it, here’s the original template
<!--
  Please check the needed checkboxes ([ ] -> [x]). Leave the
  comments as they are, they won’t show on GitHub.
  We are excited about pull requests, but please try to limit the scope, provide
  a general description of the changes, and remember, it’s up to you to convince
  us to land it.
-->

### Initial checklist

*   [ ] I read the support docs <!-- https://github.com/remarkjs/.github/blob/main/support.md -->
*   [ ] I read the contributing guide <!-- https://github.com/remarkjs/.github/blob/main/contributing.md -->
*   [ ] I agree to follow the code of conduct <!-- https://github.com/remarkjs/.github/blob/main/code-of-conduct.md -->
*   [ ] I searched issues and couldn’t find anything (or linked relevant results below) <!-- https://github.com/search?q=user%3Aremarkjs&type=Issues -->
*   [ ] If applicable, I’ve added docs and tests

### Description of changes

TODO

<!--do not edit: pr-->

Thanks,
— bb

@codecov-commenter

This comment was marked as resolved.

Copy link
Member

@wooorm wooorm left a comment

Choose a reason for hiding this comment

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

I’m open to the idea but am not sure about this approach (see inline comments).

Note that also docs and tests are missing

* @type {import('unified').Plugin<[(Options|[Severity, Options?])?, FileSet?], Root>}
* https://github.com/microsoft/TypeScript/pull/48132
* @param [rawOptions]
* @param [fileSet]
Copy link
Member

Choose a reason for hiding this comment

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

  • The typedef should not be in this block: that makes the description belong to it. The description belongs to the plugin. You can move the typedef up.
  • Severity and label are different things as types, severity does not encompass both, so I think it’s better to not import both as one name, but import them separately under their own names
  • Add type for rules' callback parameter remark-lint#283 (comment)

checkAll([file], next)
return lintRule(
{
origin: 'validate-links',
Copy link
Member

Choose a reason for hiding this comment

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

This most likely prevents ruleIds from being set. We currently use different ruleIds, and this overwrites them:

if (hash) {
reason = 'Link to unknown heading'
ruleId = constants.headingRuleId
if (base && path.join(base, filePath) !== absolute) {
reason += ' in `' + filePath + '`'
ruleId = constants.headingInFileRuleId
}
reason += ': `' + hash + '`'
} else {
reason = 'Link to unknown file: `' + filePath + '`'
ruleId = constants.fileRuleId
}
const origin = [constants.sourceId, ruleId].join(':')
.

Copy link
Member

Choose a reason for hiding this comment

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

It’s likely not correct to use use lintRule. That’s meant for one rule (one reason for emitting things). This package supports different reasons.
Perhaps coerce should be used, and exposed from unified-lint-rule?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
👋 phase/new Post is being triaged automatically
Development

Successfully merging this pull request may close these issues.

None yet

3 participants