-
Notifications
You must be signed in to change notification settings - Fork 390
chore: Enabling max-len lint rule #1014
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
Conversation
9f9ca06
to
f2ad573
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, Hiranya! LGTM!
Left one question.
@@ -246,7 +246,7 @@ describe('admin.remoteConfig', () => { | |||
INVALID_JSON_STRINGS.forEach((invalidJson) => { | |||
it(`should throw if the json string is ${JSON.stringify(invalidJson)}`, () => { | |||
expect(() => admin.remoteConfig().createTemplateFromJSON(invalidJson)) | |||
.to.throw(/^Failed to parse the JSON string: ([\D\w]*)\. SyntaxError: Unexpected token ([\D\w]*) in JSON at position ([0-9]*)$/); | |||
.to.throw(/Failed to parse the JSON string/); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did this pass integration test? I thought the regex should match the complete message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was because the regex was defined with ^...$
symbols. Without them you don't need to match the whole message.
}); | ||
}); | ||
|
||
['abc', 'a123b', 'a123', '123a', 1.2, '70.2'].forEach( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! :)
}); | ||
}); | ||
|
||
['abc', 'a123b', 'a123', '123a', 1.2, '70.2'].forEach((invalidVersion) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! :)
I'm enabling the
max-len
lint rule with a max line length of 120. Ideally we should be using a max line length of 100, but we have way too many violations to attempt that right now.