Could we put lint messages in rule metadata?

[platinumazure]

Inspired by #6738, where it was noticed that some of our lint messages are inconsistent about ending in periods.

I think it would be really awesome to have lint messages as a part of rule metadata. This does not have immediate benefit for linting, but it would allow us to do interesting things with auto-generated documentation, and even allow searching by error message on the site.

Proposed API

We would support a new messages key on rule meta objects. The value of this key is an object with arbitrary keys and with string values equal to different messages being used.

Examples

// lib/rules/array-bracket-spacing.js
module.exports = {
    meta: {
        docs: {
            description: "enforce consistent spacing inside array brackets",
            category: "Stylistic Issues",
            recommended: false
        },
        fixable: "whitespace",
        schema: [
            {
                enum: ["always", "never"]
            },
            {
                type: "object",
                properties: {
                    singleValue: {
                        type: "boolean"
                    },
                    objectsInArrays: {
                        type: "boolean"
                    },
                    arraysInArrays: {
                        type: "boolean"
                    }
                },
                additionalProperties: false
            }
        ],
        messages: {
            noBeginningSpace: "There should be no space after '{{ token }}'",
            noEndingSpace: "There should be no space before '{{ token }}'",
            requiredBeginningSpace: "A space is required after '{{ token }}'",
            requiredEndingSpace: "A space is required before '{{ token }}'"
        }
    }
    // create: function (context) ...
};

// lib/rules/no-continue.js
module.exports = {
    meta: {
        docs: {
            description: "disallow `continue` statements",
            category: "Stylistic Issues",
            recommended: false
        },

        schema: [],

        messages: {
            default: "Unexpected use of continue statement"
        }
    },
    // create: function (context) ...
};

Example usage:

context.report({
    node: node,
    messageId: "noSpacesBefore",
    data: {
        token: token
    }
});

Challenges

The main challenge is figuring out how to access the messages from within the rule visitor functions, where we typically don't use object-oriented patterns. Very worst case (I'm hoping to avoid this), maybe we could augment RuleContext to support a getMessages() function which can return the available messages.

A smaller challenge is to convert messages that use string concatenation (see array-bracket-spacing source for an example) to use our token replacement engine instead. I assume this is not a significant roadblock.

Benefits

Here are the benefits I can see with doing this (none of which have to do with actual linting):

1. Make it easier for information architect types like @pedrottimark or @scriptdaemon or @aubergine10 to find lint messages and ensure consistency across all core rules
2. Rule documentation auto-generation could include a block showing the possible linting messages for a rule, so that a user knows how violations will actually look (notwithstanding placeholder tokens which depend on the violation context).
3. Rule linting messages could be indexed in the site search. That way a user could search for the error message they are getting, and we can pull up the rule generating that message. (Anyone remember jslinterrors.com?)

Etc.

What do folks think? Am I missing any significant challenges or roadblocks? Have I overstated the potential benefits? Or could this be worth pursuing at some point?

[nzakas]

This was discussed as part of the original metadata proposal and was dropped due to the complexity of referencing those strings from within the rule body (search for "meta" in closed PRs -- I'm sure you'll find those old discussions).

We saw the same benefits of doing this, but the logistics ended up getting in the way, so we abandoned that part of the proposal. We even discussed something like context.getMessage but felt like that was making rule creators jump through too many hoops.

[platinumazure]

I should have known I wasn't the first to think of this. Sorry about that!

I would like to suggest one more possible implementation that might be less onerous to developers:

• Rule metadata still has an optional messages key. In this proposal, meta.messages must be an object if it is provided.
• context.report() accepts a messageId property, which must be one of the keys in the messages object.
  • The report function will use the rule metadata to substitute a message, and optionally apply token substitution to the message.
  • The report function should throw if message and messageId are both specified.
  • The report function should throw if messageId is supplied and rule is not new-style or the message key does not match any keys in meta.messages.

This seems less intrusive to me, but if even this approach is too intrusive or cumbersome, then I'm happy to close this issue. Thanks for your consideration.

[mysticatea]

messageId sounds good to me.
It may make easy supporting i18n.

[nzakas]

Sounds rational. @ilyavolodin thoughts?

[ilyavolodin]

Sounds good to me. I would love to move messages into metadata. My only concern is that we have to support both IDs and strings (for backwards compatibility), in context.report, so there's no way to enforce this, other then with another rule (which is not a bad idea).
Also, when I prototyped adding messages to metadata, @nzakas has a concern that metadata comes first, and report comes late in the code. User might not know upfront what message they will need. I don't think it's a huge problem, but something to think about.
One other thing to keep in mind, if we want to support i18n at some point, we would need mechanism to extract those message and substitute them with something else. Nothing that we have to worry about right now, but just something to keep in mind.

[BYK]

One other thing to keep in mind, if we want to support i18n at some point, we would need mechanism to extract those message and substitute them with something else. Nothing that we have to worry about right now, but just something to keep in mind.

Wrapping those strings in gettext calls should do quite a lot for this.

[platinumazure]

I think I count 3 approvals (@mysticatea, @nzakas, @ilyavolodin) and I'm willing to champion and implement.

Regarding i18n, I propose that we agree to try to implement in an i18n friendly manner, but that we do not attempt to include i18n in this specific proposal or to choose a particular library for it in this discussion.

Regarding potential implementation, I would propose something like this:

• Semver-minor PR to support messageId and meta.messages and implement in one or two rules
• Semver-patch PR to update documentation for context.report
• One or more semver-patch PRs to convert our rules to use this new style of message if all goes well
• One semver-minor (?) PR to create a new internal rule enforcing new style messages (pending feedback on usability for rule developers)

Does this seem like a sensible approach? Am I missing anything?

[ilyavolodin]

Documentation update should go with step 2 as well.
Also can you update original proposal with messageIds? Just so we would have an example of the structure.

@sindresorhus Any specific concerns about moving message into metadata? That would enable us to eventually have i18n support and probably improve documentation, but automatically appending all possible message to each individual article (should help with search results).

[platinumazure]

@ilyavolodin:

1. Regarding documentation step, edited accordingly.
2. Regarding amending the original post to reflect the new proposal: Good call, I'll try to get to that today. Done.

[nzakas]

Just as a reminder, this is really a "nice to have," so if you have some free time, it would be great to use that on the JSCS tasks first. We can always come back to this at a later time.

(Not trying to detail, we just really need to focus our attention, both coding and reviewing, on the highest priority stuff first.)

[platinumazure]

Fair enough @nzakas, I'll re-evaluate when I get back in town. Perhaps you
would like to add a "backlog" label or similar to indicate that it is
currently a lower priority? Just a suggestion. Thanks either way.

On Jul 26, 2016 8:37 AM, "Nicholas C. Zakas" notifications@github.com
wrote:

Just as a reminder, this is really a "nice to have," so if you have some
free time, it would be great to use that on the JSCS tasks first. We can
always come back to this at a later time.

(Not trying to detail, we just really need to focus our attention, both
coding and reviewing, on the highest priority stuff first.)

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#6740 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AARWeiQZsqZ2HUflSnqxNX0episKsvRkks5qZjfIgaJpZM4JSc4f
.

[platinumazure]

@eslint/eslint-tsc I've put a "help wanted" on this issue since it's a low priority for core team but could still be worth doing. Please remove the label if this is not desired. Thanks!

[platinumazure]

TSC Summary: This issue is accepted, but I wanted to see if this merited being included on the core roadmap project.

TSC Question: Should this issue be added to the core roadmap project?

[alberto]

@platinumazure what's the implication of being in the core roadmap? Why do you think this (and your other proposals) should be included?

[platinumazure]

@alberto To me, it's a matter of asking if we want to consider these as important for the next big milestone (e.g., do we want these done in core as part of 4.0). The "core roadmap" project was added and some issues were added to that project with a not-very-transparent process, and I wanted to put these up as potential candidates. If the TSC decides they do not belong on a core roadmap, then at least it's documented via TSC meeting notes and in this issue.

So this is also an opportunity for TSC to gain awareness of these potential core roadmap-worthy issues, even if the ultimate outcome is not to put them on the core roadmap project.

As for why these should or shouldn't be in the core roadmap, that comes down to the merits of each issue (i.e., what change is proposed in each issue) and evaluating whether or not that meshes with the vision of the project and/or is important enough to schedule for the next major version milestone. I believe the issues should be considered for inclusion, but not necessarily actually included.

Make sense?

[kaicataldo]

As per the discussion during today's TSC meeting, this issue has been added to the Core Roadmap.

[j-f1]

An addition to this could be the ability for RuleTester to accept error objects in the invalid section with messageId and data, reducing code duplication.

[j-f1]

I’m starting work on this as outlined in #6740 (comment).