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

user defined error message #7

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

user defined error message #7

wants to merge 1 commit into from

Conversation

kwang-aa
Copy link

@kwang-aa kwang-aa commented Mar 6, 2017

Hi Stuk, we are using this header checker plugin and for some reason we need to define the error message by ourselves.
So I expand another argument to store it.

Copy link
Owner

@Stuk Stuk left a comment

Choose a reason for hiding this comment

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

Is this related to #6 ? If so, have you considered any of the feedback I gave?

code: "/* Copyright 20\n Author: [email protected] */",
options: ["block", {pattern: "^\\s*Copyright \\(c\\) \\d{4} ABC Inc. All rights reserved.\\s*$"}, "Copyright information error. Please add below line to your file.\n\tCopyright (c) " + new Date().getFullYear() + " ABC Inc. All rights reserved"],
errors: [
{message: "Copyright information error. Please add below line to your file.\n\tCopyright (c) " + new Date().getFullYear() + " ABC Inc. All rights reserved"}
Copy link
Owner

Choose a reason for hiding this comment

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

These tests will fail in 2018.

Choose a reason for hiding this comment

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

@Stuk, maybe I am missing something, why would this fail in 2018?

The pattern does not match the code, which would cause it to throw an error. And the supplied error message would match the output since new Date().getFullYear() is used in both places.

Copy link

Choose a reason for hiding this comment

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

@Stuk do you mean it will fail in 2100 ?

@iowillhoit
Copy link

Regarding #6, I agree that avoiding duplication would be ideal. But until a more elegant solution is submitted, I think this should be merged.

I would love to be able to use this plugin, but I'm not going to confuse our 100+ engineers with an ambiguous error missing header (header/header) on (almost) every file.

Worst case scenario, if someone has a complicated header, they could link to a wiki page or a gist in the eslint error message that contains the full header.

@pranaygp
Copy link
Collaborator

pranaygp commented Oct 18, 2017

just chiming in here, but it seems to me like the only reason you might need to duplicate to an expectedHeader field of some sort is if you're using a regex matcher.

In such a case, would it make more sense to convert the regexps into strings (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/RegExp/toString)?

that way, if you're not using a regular expression, it's easy because you can output the correct string. if you're using a regular expression (in the case of an array, atleast one regular expression), you can iterate through the array and concat the lines together, and in the case you get a regexp, make it a string before joining them?

You could even surround the regexp strings with something like {{...}} to make it obvious to an engineer what they're meant to do?

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

Successfully merging this pull request may close these issues.

5 participants