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

Use get for all Dust references #360

Merged
merged 7 commits into from
Nov 8, 2013
Merged

Conversation

smfoote
Copy link
Contributor

@smfoote smfoote commented Nov 5, 2013

dot-notation references will now use get, and getPath is deprecated.

Also in this commit:

  • Update to version 2.2.0
  • When get returns a function, add an isFunction attribute to the returned function and set it to true. This fixes the bug that was causing dustjs-helpers tests to fail.

dot-notation references will now use get, and getPath is deprecated.

Also in this commit:

* Update to version 2.2.0
* When get returns a function, add an `isFunction` attribute to the returned function and set it to true. This fixes the bug that was causing dustjs-helpers tests to fail.
@smfoote
Copy link
Contributor Author

smfoote commented Nov 5, 2013

Currently merging changes. Will update PR shortly.

@@ -302,7 +321,7 @@ dust.nodes = {
},

key: function(context, node) {
return "ctx.get(\"" + node[1] + "\")";
return "ctx.get(false, [\"" + node[1] + "\"])";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is one of the lines that actually changed.

@prashn64
Copy link
Contributor

prashn64 commented Nov 6, 2013

lgtm, let's get another pair of eyes on it before we merge.

@rragan
Copy link
Contributor

rragan commented Nov 6, 2013

Let me step back and ask why do this?

get/getPath is a mostly internal interface (barring helpers) between the compiler and the dust runtime. Other than refactoring to consolidate to one routine what does this gain us? I guess it might slow down get and getPath each a bit as they were specialized for their own cases before. It's not like we can get rid of getPath either for a long time due to use by helpers so download size will presumably get larger.

@smfoote
Copy link
Contributor Author

smfoote commented Nov 6, 2013

The reason for this change is actually based on a bug in the dustjs-helpers repo. As we were trying to debug, we tracked the problem down to the get method. However, the method was very difficult to understand as it was written. As I rewrote it, I found the source of the bug (PR to be added to the dustjs-helpers repo after this PR is merged).

The new get method only adds a few steps beyond what the previous get method included for references without dots. It is still optimized for the no-dot reference use case.

Finally, the new version will reduce the total amount of code, because the getPath method now just points to the get method.

In summary, this change (1) makes get easier to understand and maintain, (2) fixes a bug in dustjs-helpers, (3) reduces the total size of the library (slightly), and (4) is backward compatible.

@jimmyhchan
Copy link
Contributor

does this pass the tests added in #357 it would be good to fix this once.

@rragan
Copy link
Contributor

rragan commented Nov 7, 2013

See also my comment/proposed fix on #340. This should also be verified to be OK.

@smfoote
Copy link
Contributor Author

smfoote commented Nov 7, 2013

I added the tests from #357 and all tests pass. I think the issue in #340 is separate enough that it should be a different pull request.

@rragan
Copy link
Contributor

rragan commented Nov 7, 2013

Thanks for covering #357. Agree #340 should be separate

@jimmyhchan
Copy link
Contributor

As discussed let's address a few issues:

  • context.get('key') should be the API ... having to pass false as the first argument is not nice.
  • should we support context.get('key.with.dots')

I think we agree the API should be intuitive and simple and the compiler output should be fast and direct..... not sure what the best solution is.

Steven Foote added 2 commits November 7, 2013 20:41
The public API (to be used in helpers) now supports getting in the following ways:

'key'
'.key'                   // Only searches in the current context
'path.to.key'
'.path.to.key'           // Only searches starting in the current context
['key']
['path', 'to', 'key']
Go straight from getPath to _get

Also, update documentation for _get
@jimmyhchan
Copy link
Contributor

the dist file seems to be out of date. did you make?

@jimmyhchan
Copy link
Contributor

thanks for the fixes. This looks good.
@rragan how does this look to you?

@rragan
Copy link
Contributor

rragan commented Nov 8, 2013

Just this one question:

In _get, this bit of code:
if (typeof arguments[0] === 'string') {
// For backwards compatability, if the first argument is a string, make that string the
// first item in the down array.
down = [arguments[0]];
cur = false;
}

_get is a new private function so presumably no one is calling it directly from old code. The actual get function, seems to ensure cur is a boolean.
When would the if test for "string" ever be true? Legacy getPath always had a boolean as the first argument too (unless there is some edge case I don't remember). Is this code needed?

@smfoote
Copy link
Contributor Author

smfoote commented Nov 8, 2013

Good point. It's not needed anymore.

prashn64 added a commit that referenced this pull request Nov 8, 2013
Use get for all Dust references
@prashn64 prashn64 merged commit ae64e83 into linkedin:master Nov 8, 2013
@smfoote smfoote deleted the getPath2get branch February 2, 2015 23:32
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.

4 participants