Skip to content

Sort linter output#806

Merged
sebastianbenz merged 7 commits intoampproject:masterfrom
Dbrown910:sortLinterOutput
May 29, 2020
Merged

Sort linter output#806
sebastianbenz merged 7 commits intoampproject:masterfrom
Dbrown910:sortLinterOutput

Conversation

@Dbrown910
Copy link
Copy Markdown
Contributor

/^(googlebot_desktop|googlebot_mobile|chrome_desktop|chrome_mobile)$/i,
"googlebot_mobile"
)
.option(`-n, --no-supress`, "supress passing tests in output")
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The description seems to contradict the parameter name (supress vs no supress).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

whoops typo. I did change this but forgot to commit the edit. Will fix


function lint(args, logger) {
return cli(['dummy'].concat(args), logger, 'amp lint'); // "dummy" to simulate process.argv
return cli(['dummy'].concat(args).concat(['-n']), logger, 'amp lint'); // "dummy" to simulate process.argv
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Am I right that this will by default supress the passing tests? How do I print the passing tests? Do I need to set -n=false? I'd find it to be more intuitive to name the attribute something like --show-passing which is disabled by default.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

-n will unsupress the passing tests since they are hidden by default. I had to add this flag here to pass the toolbox testing suite, as one of its checks is "has at least one passing test".
I can change the flag to -s, --show-passing

Copy link
Copy Markdown
Collaborator

@sebastianbenz sebastianbenz left a comment

Choose a reason for hiding this comment

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

nice!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants