Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

Improve failure message & docs for ban-comma-operator rule (#3380) #3384

Merged
merged 2 commits into from
Oct 24, 2017
Merged

Improve failure message & docs for ban-comma-operator rule (#3380) #3384

merged 2 commits into from
Oct 24, 2017

Conversation

ChrisMBarr
Copy link
Contributor

PR checklist

Overview of change:

This improves the failure message, improves the description metadata, and adds rationale in the metadata for the ban-comma-operator based on the discussions in #3380

Is there anything you'd like reviewers to focus on?

CHANGELOG.md entry:

[enhancement] ban-comma-operator - improved failure message and documentation

Copy link
Contributor

@ajafff ajafff left a comment

Choose a reason for hiding this comment

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

LGTM with the comment addressed

public static metadata: Lint.IRuleMetadata = {
ruleName: "ban-comma-operator",
description: "Bans the comma operator.",
description: "Disallows the comma operator to be used. [Read more about the comma operator here](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Comma_Operator).",
Copy link
Contributor

Choose a reason for hiding this comment

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

In the rules list this text will be used as link to the rule's documentation page.
We should avoid putting a link inside another link.

Consider using descriptionDetails instead.

Copy link
Contributor Author

@ChrisMBarr ChrisMBarr Oct 24, 2017

Choose a reason for hiding this comment

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

Ah good point - I forgot about that! Just pushed a fix for this.

to avoid nested links in the docs index
@ajafff ajafff merged commit 1365c3c into palantir:master Oct 24, 2017
@ajafff
Copy link
Contributor

ajafff commented Oct 24, 2017

Thanks @ChrisMBarr

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

Successfully merging this pull request may close these issues.

2 participants