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

Make the Keypather class auto-instance. #10

Closed
wants to merge 5 commits into from

Conversation

justsml
Copy link
Contributor

@justsml justsml commented Feb 24, 2015

Hey @tjmehta I love your code's approach & scope of feature support... Anyway I will create a separate issue to discuss some of my thoughts. Anyway, here's a patch to eliminate the need for double parenthesis:
See my updates to the README.md

@justsml
Copy link
Contributor Author

justsml commented Feb 24, 2015

Hey I wasn't sure how I broke it... I thought the other option 'force' tests would fail if I broke the option...

I'm trying to detect options if supplied when explicitly calling into the Keypather(opts) class,
Regardless of whether or not options were passed, I wanted to try an auto-instance pattern, something like this: if (!(this instanceof Keypather)) return new Keypather(opts) in the index.js. Hopefully eliminating the double sets of parens needed.

this.force = (force !== undefined) ? Boolean(force) : true; // force - default: true
function Keypather (opts) {
if (!(this instanceof Keypather)) { return new Keypather(opts); }// Auto instantiate
this.force = (opts && opts.force && Boolean(opts.force) === false ? false : true); // force - default: true

Choose a reason for hiding this comment

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

This should be:
this.force = (opts && typeof opts.force !== 'undefined' && Boolean(opts.force) === false ? false : true);
else it's always true.

@stoeffel
Copy link

This wouldn't work since you have to create a instance of keypather somewhere.

var keypath = require('keypather');

// keypath is a function

 var obj = {
  ...
 };
 keypath.get(obj, "foo.bar.baz"); // This would break get is not a function of keypath.

@justsml
Copy link
Contributor Author

justsml commented Feb 24, 2015

Hey @stoeffel - Hey thanks for the reply.
What if the Keypather function itself was 'decorated' like this: (is that the term?)

// Instead of setting the method on the prototype, something like this:
Keypather.get = function() { /* ... */ }

I'm not 100% sure, but is this a valid pattern to allow this kind of usage:
Keypather.prototype.get = Keypather.get = function() { /* ... */ }

@tjmehta
Copy link
Owner

tjmehta commented Feb 25, 2015

Hmm, maybe this:

module.exports = factoryAndInstance;
function factoryAndInstance (opts) {
  var keypather = new Keypather(opts && opts.force);
  return keypather;
};
Keypather.call(factoryAndInstance);
factoryAndInstance.__proto__ = new Keypather().__proto__;

@tjmehta
Copy link
Owner

tjmehta commented Feb 25, 2015

^ that would export a functioning keypather instance defaulted with force:true

@stoeffel stoeffel mentioned this pull request Feb 25, 2015
@justsml
Copy link
Contributor Author

justsml commented Feb 25, 2015

Hey guys... I updated the code... It's still failing... I was wondering how important having the force option is? It adds a layer of branching in the code that might be handled a little differently... I was thinking of moving this kind of functionality under something like keypath.trySet() otherwise you have to choose up front how every keypather call will be affected. ... @tjmehta @stoeffel any thoughts? ... Sorry if this is kinda drifting off topic...

this.force = (force !== undefined) ? Boolean(force) : true; // force - default: true
function Keypather (opts) {
if (!(this instanceof Keypather)) { return new Keypather(opts); }// Auto instantiate
this.force = (opts && typeof opts.force !== 'undefined' && Boolean(opts.force) === false ? false : true);
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think you need these modifications just revert it to what it was
this.force = (force !== undefined) ? Boolean(force) : true; // force - default: true

Thanks @tjmehta ... I forgot to roll that back... Keypather() is back to the way it was
@justsml
Copy link
Contributor Author

justsml commented Feb 27, 2015

Just updated... Woo hoo!
Thanks guys

@stoeffel
Copy link

Travis is still failing. You should run the test before pushing (npm test).
And you should update the tests to check if it works without instantiating keypather.

@justsml
Copy link
Contributor Author

justsml commented Mar 13, 2015

Thanks @stoeffel - I was loading broken stuff for yours & @tjmehta 's feedback, sorry if it should have taken place in an Issue or wiki or whatever...
I'll get time this weekend to load a bunch of updates/ideas...

@tjmehta
Copy link
Owner

tjmehta commented Jul 13, 2015

Going to close this for now, feel free to reopen if you continue work on this.

@tjmehta tjmehta closed this Jul 13, 2015
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.

3 participants