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

feature: add support for named constructors #180

Merged

Conversation

OJezu
Copy link
Contributor

@OJezu OJezu commented Mar 29, 2016

Adds support for named constructors in "create" factory.

I would love to add tests, but I did not find tests for the "create" factory itself.

This PR also allows "hack" where not-a-module is passed as module. It can be then used as factory. Maybe name of "constructorName" option should reflect this, and be "method" instead? And add "factory" as an alias for "module"?

@briancavalier
Copy link
Member

Thanks, @OJezu! I should have time to look at this in the next couple days.

@briancavalier
Copy link
Member

This looks good @OJezu. Could you add unit tests for the new namedConstructor feature? Thanks!

@OJezu
Copy link
Contributor Author

OJezu commented Apr 3, 2016

@briancavalier I will get on those tests in two-three days.

@briancavalier
Copy link
Member

Great, thank you.

@OJezu OJezu force-pushed the feature/named-constructor-support branch from 2c75f65 to f2a3b61 Compare April 6, 2016 21:03
@OJezu OJezu force-pushed the feature/named-constructor-support branch from f2a3b61 to 230d16a Compare April 6, 2016 21:06
@OJezu
Copy link
Contributor Author

OJezu commented Apr 6, 2016

@briancavalier I've added tests. I might have been a little drunk when writing them, so I am even more open to criticism than usual.

I've also added some more recent versions of node.js to .travis.yml, I hope it's not a problem.

@briancavalier
Copy link
Member

@OJezu great, thank you. I'll take a look in the next couple days.

@briancavalier
Copy link
Member

Tests look good, @OJezu, thank you!

I've also added some more recent versions of node.js to .travis.yml, I hope it's not a problem.

Not a problem at all. I appreciate it.

@briancavalier briancavalier merged commit 4185eb1 into cujojs:master Apr 10, 2016
@briancavalier
Copy link
Member

I'll get a release ready soon.

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