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

Allow building types with .from using an object #252

Merged
merged 1 commit into from
Mar 9, 2018

Conversation

elliottsj
Copy link
Contributor

With .from, you can be explicit about which fields you want to
populate, without having to deal with the positional arguments
of the normal builder.

See also:
facebook/jscodeshift#180 (comment)

@elliottsj
Copy link
Contributor Author

Here's my first stab at implementing the .from builder; let me know what you think

@benjamn
Copy link
Owner

benjamn commented Feb 16, 2018

Looking forward to reviewing this weekend!

lib/types.js Outdated
// using field values from the passed object. For fields missing from the passed object,
// their default value will be used.
builder.from = function (obj) {
var built = Object.create(nodePrototype);
Copy link
Owner

Choose a reason for hiding this comment

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

This object creation can come after the self.finalized check, though I guess the same could be said for the existing code in the builder function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

lib/types.js Outdated

Object.keys(self.allFields).forEach(function (param) {
// Use the default value.
addParam(built, param, null, false);
Copy link
Owner

Choose a reason for hiding this comment

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

I think this loop also needs to check whether obj[param] is defined, so that obj can provide values for more than just the .build parameters, e.g.

builders.identifier.from({ name: "foo", typeAnnotation: ... })

The typeAnnotation parameter isn't one of the .build parameters, but it seems like it should get checked and included, if provided.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, you're right. I think we don't even need to loop over self.buildParams either since any field in self.allFields could be specified in obj. Now updated.

With `.from`, you can be explicit about which fields you want to
populate, without having to deal with the positional arguments
of the normal builder.

See also:
facebook/jscodeshift#180 (comment)
Copy link
Owner

@benjamn benjamn 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!

@benjamn benjamn merged commit ea80bd2 into benjamn:master Mar 9, 2018
@elliottsj elliottsj deleted the from branch March 18, 2018 19:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants