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

Add --no-diff option (fixes #2514) #2536

Closed
wants to merge 3 commits into from
Closed

Add --no-diff option (fixes #2514) #2536

wants to merge 3 commits into from

Conversation

CapacitorSet
Copy link
Contributor

Adds a --no-diff flag, which disables diffs completely (takes priority over --inline-diffs). Made to fix #2514.

Copy link
Contributor

@boneskull boneskull left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I have a few requested changes.

I think we should use the singular "diff" in any relevant object property names and CLI flags; what do you think?

Can you please add a test within test/reporters/base.spec.js? It'd also be good to create a new file as test/integration/no-diff.spec.js.

Otherwise this looks good. 👍

bin/_mocha Outdated
@@ -242,6 +243,12 @@ if (program.inlineDiffs) {
mocha.useInlineDiffs(true);
}

// --no-diffs

if (process.argv.indexOf('--no-diffs') !== -1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this doesn't match the actual option

bin/_mocha Outdated
// --no-diffs

if (process.argv.indexOf('--no-diffs') !== -1) {
mocha.doNotShowDiffs(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

would prefer hideDiffs() or disableDiffs() or something

lib/mocha.js Outdated
@@ -509,6 +523,7 @@ Mocha.prototype.run = function (fn) {
exports.reporters.Base.useColors = options.useColors;
}
exports.reporters.Base.inlineDiffs = options.useInlineDiffs;
exports.reporters.Base.noDiffs = options.noDiffs;
Copy link
Contributor

Choose a reason for hiding this comment

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

ugh, this sucks (not your fault)

@@ -212,7 +212,9 @@ exports.list = function (failures) {
var match = message.match(/^([^:]+): expected/);
msg = '\n ' + color('error message', match ? match[1] : msg);

if (exports.inlineDiffs) {
if (exports.noDiffs) {
Copy link
Contributor

Choose a reason for hiding this comment

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

please avoid the empty block:

if (exports.inlineDiffs) {
  msg += inlineDiff(err, escape);
} else if (!exports.noDiffs) {
  msg += unifiedDiff(err, escape);
}

mocha.js Outdated
@@ -1641,6 +1641,20 @@ Mocha.prototype.useInlineDiffs = function (inlineDiffs) {
};

/**
* Do not show diffs at all.
Copy link
Contributor

Choose a reason for hiding this comment

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

please don't commit changes to this file. it's lame we keep it under VCS (b/c bower)

@boneskull boneskull added type: feature enhancement proposal pr-needs-work labels Oct 16, 2016
@boneskull boneskull added this to the v3.2.0 milestone Oct 16, 2016
@CapacitorSet
Copy link
Contributor Author

Done! I squashed the changes as recommended in CONTRIBUTING.md.

@CapacitorSet
Copy link
Contributor Author

Ping @boneskull. This PR still has the pr-needs-work label, but I think it's fixed now.

@boneskull boneskull modified the milestone: v3.2.0 Nov 24, 2016
@rhali786
Copy link

Hey did this get pulled out? I don't see it in the code. I was looking forward to this.

@boneskull boneskull added status: needs review a maintainer should (re-)review this pull request and removed pr-needs-work labels Oct 11, 2017
@boneskull
Copy link
Contributor

someone needs to review this and probably resolve the conflicts. as to how soon that'll happen, I can't tell you

boneskull added a commit that referenced this pull request Dec 12, 2017
boneskull added a commit that referenced this pull request Dec 12, 2017
boneskull added a commit that referenced this pull request Dec 12, 2017
boneskull added a commit that referenced this pull request Dec 12, 2017
boneskull added a commit that referenced this pull request Dec 12, 2017
boneskull added a commit that referenced this pull request Dec 12, 2017
@boneskull
Copy link
Contributor

closed by 7ceaaf2 and 575d83d

@boneskull boneskull closed this Dec 12, 2017
@boneskull boneskull added semver-minor implementation requires increase of "minor" version number; "features" and removed status: needs review a maintainer should (re-)review this pull request labels Dec 12, 2017
sgilroy pushed a commit to TwineHealth/mocha that referenced this pull request Feb 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-minor implementation requires increase of "minor" version number; "features" type: feature enhancement proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Option to disable diff
3 participants