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

Bug 1505758 - Fix/enable several more ESLint rules #4245

Merged
merged 13 commits into from Nov 8, 2018
Merged

Conversation

edmorley
Copy link
Contributor

@edmorley edmorley commented Nov 8, 2018

See individual commits for easier review and links to the ESLint rule docs for each rule.

I've mostly left the Perfherder/log viewer files untouched (using eslint-disable there instead) so as to reduce churn/potential for bugs given they are going to be rewritten soon anyway. The main objective is to ensure our existing newly rewritten JS and even more importantly any about-to-be-written JS has full rule coverage.

@edmorley edmorley self-assigned this Nov 8, 2018
Copy link
Contributor

@camd camd left a comment

Choose a reason for hiding this comment

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

This is so awesome. I feel pangs of guilt when I see new classes I created that have cruft in there. But at least now the linter will help keep me honest. :)

@@ -78,6 +78,8 @@ export default class JobButtonComponent extends React.Component {
const resultStatus = state === 'completed' ? result : state;
const runnable = state === 'runnable';
const btnClass = getBtnClass(resultStatus, failure_classification_id);
// TODO: This is broken (bug 1504713)
// eslint-disable-next-line no-restricted-globals
let title = `${resultStatus} | ${job_type_name} - ${status}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can take - ${status} out of the template here. I can't think of what it was supposed to be at this point, since resultStatus covers both result and state.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I agree - I've left a comment on bug 1504713 :-)
(Won't fix here so as to not introduce behaviour changes in this PR)

]).then(async (results) => {

// The first result comes from the job promise.
]).then(async ([jobResult, jobDetailResult, jobLogUrlResult, phSeriesResult]) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice... this is a lot clearer now.

{autoclassifyStatus === 'failed' && <span>Autoclassification failed</span>}
</div>}
{
// TODO: This is broken (bug 1504711)
Copy link
Contributor

Choose a reason for hiding this comment

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

tbh: I'm not certain what the fix is for this. I suppose we'll have to cross this bridge when we address re-enabling the autoclassify tab...

@edmorley edmorley merged commit 69501e0 into master Nov 8, 2018
@edmorley edmorley deleted the eslint-enable-more branch November 8, 2018 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants