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.

Sign up for GitHub

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

Jump to bottom

[enhancement] #960 allow debug() to exclude props #961

Merged
merged 1 commit into from
Jun 12, 2017
Merged

[enhancement] #960 allow debug() to exclude props #961

merged 1 commit into from
Jun 12, 2017

Conversation

finnigantime
Copy link
Contributor

@finnigantime finnigantime commented May 31, 2017 •
edited
Loading

Implements #960. Adds options object to debug(). Uses options.ignoreProps to opt-in to ignoring props from the debug() string output. This change is backwards-compatible so existing usages of debug() are unaffected.

@finnigantime finnigantime mentioned this pull request May 31, 2017
Merged
ljharb
ljharb requested changes May 31, 2017
View reviewed changes
src/Debug.js Outdated
let beforeProps = '';
if (!options.ignoreProps) {
props = propsString(node);
beforeProps = props ? ' ' : '';
Copy link
Member

Choose a reason for hiding this comment

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

I think i'd prefer to change line 72 to const props = options.ignoreProps ? '' : propsString(node); and then none of the consts need to change to lets.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok sure, I will change this to keep these using const.

src/Debug.js Outdated
export function debugNodes(nodes) {
return nodes.map(node => debugNode(node)).join('\n\n\n');
export function debugNodes(nodes, options) {
return nodes.map(node => debugNode(node, undefined, options)).join('\n\n\n');
Copy link
Member

Choose a reason for hiding this comment

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

This should probably be options = {}, since it's not a required argument

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. I defaulted it to empty object in debugNode(), but it makes the code more consistent to default it to empty object here too. Will do.

src/ShallowWrapper.js Outdated
* @returns {String}
*/
debug() {
return debugNodes(this.getNodes());
debug(options) {
Copy link
Member

Choose a reason for hiding this comment

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

Since this is optional, it should be defaulted (= {}), so it doesn't contribute to the function's length.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do. What do you mean by contributing to the function's length?

Copy link
Member

Choose a reason for hiding this comment

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

function (options) {}.length is 1, function (options = {}) {}.length is 0 - a function's "length" property tells you how many required arguments it has.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes total sense! Thanks for explaining.

@finnigantime
Copy link
Contributor Author

Feedback addressed.

@finnigantime finnigantime reopened this Jun 1, 2017
ljharb
ljharb reviewed Jun 1, 2017
View reviewed changes
src/Debug.js Outdated
const nodeClose = childrenStrs.length ? `</${type}>` : '/>';

const props = options.ignoreProps ? '' : propsString(node);
const beforeProps = (options.ignoreProps || !props) ? '' : ' ';
Copy link
Member

Choose a reason for hiding this comment

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

if the ignoreProps option is set, then props will be the empty string, which is falsy - i think you can just revert this line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Cleaned this up. Let me know if I should squash or of it's fine to have this as an additional commit.

Copy link
Member

Choose a reason for hiding this comment

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

Either's OK, but it's always better to rebase down to atomic commits :-)

ljharb
ljharb approved these changes Jun 1, 2017
View reviewed changes
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

This LGTM, but I'm concerned that now the shallow debug API will deviate from that of mount.

Could we enhance ReactWrapper's "debug" output to support the same options?

@finnigantime
Copy link
Contributor Author

finnigantime commented Jun 1, 2017 •
edited
Loading

Sure, that seems reasonable. I'll be out a week, but when I return I could do that either as part of this PR or a separate one. For the CI failure, I see:

01 06 2017 01:48:41.764:ERROR [framework.browserify]: bundle error
01 06 2017 01:48:41.766:ERROR [framework.browserify]: Error: Cannot find module 'react/addons' from

Doesn't seem related to this change. Is there a way I can try kicking off another build?

Edit: I see all 3 builds from this branch have failed. Do you know what the issue is here?

@ljharb
Copy link
Member

ljharb commented Jun 1, 2017

I think it's related to npm v5 issues. I'll rebase your branch on top of my fixes and that should do it.

@ljharb ljharb force-pushed the finnigantime/ignoreProps branch 2 times, most recently from b9d2571 to cbae676 Compare June 1, 2017 08:16
@finnigantime
Copy link
Contributor Author

@ljharb Added support for ReactWrapper.

ljharb
ljharb approved these changes Jun 10, 2017
View reviewed changes
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Thanks, this looks great!

I'm wondering if in addition to true, we might want this option to also support an array of prop names to ignore - that way you could ignore className but nothing else, for example. That can be a different PR, tho :-)

@ljharb
Copy link
Member

ljharb commented Jun 10, 2017

@finnigantime once tests are passing, i'll add other collabs as reviewers so we can get this in. It looks like they're failing because jsdom fails pre-iojs, and mount only works with jsdom. Perhaps the shallow and mount assertions should be split into separate it tests, and you can use describeWithDOM to wrap the mount ones?

nfcampos
nfcampos requested changes Jun 11, 2017
View reviewed changes
Copy link
Collaborator

@nfcampos nfcampos left a comment

Choose a reason for hiding this comment

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

Looks great, but could you update the docs for debug?

@finnigantime
Copy link
Contributor Author

@nfcampos Thanks for pointing that out. Done!

@ljharb ljharb merged commit 91bf861 into enzymejs:master Jun 12, 2017
@finnigantime finnigantime deleted the finnigantime/ignoreProps branch June 12, 2017 20:48
@finnigantime finnigantime mentioned this pull request Jan 24, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ljharb ljharb ljharb approved these changes

@nfcampos nfcampos nfcampos approved these changes

@lelandrichardson lelandrichardson Awaiting requested review from lelandrichardson

@blainekasten blainekasten Awaiting requested review from blainekasten

@aweary aweary Awaiting requested review from aweary

Assignees
No one assigned
Labels
API: mount API: shallow semver: minor New stuff.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@finnigantime @ljharb @nfcampos