Skip to content

Use Liquid::Expression::LITERALS to look up parsed literal names#23

Merged
pushrax merged 1 commit into
masterfrom
use-expression-literal-map
Jun 6, 2015
Merged

Use Liquid::Expression::LITERALS to look up parsed literal names#23
pushrax merged 1 commit into
masterfrom
use-expression-literal-map

Conversation

@pushrax
Copy link
Copy Markdown
Contributor

@pushrax pushrax commented Jun 5, 2015

pushrax
pushrax reviewed Jun 5, 2015
View reviewed changes
Comment thread ext/liquid_c/parser.c
Copy link
Copy Markdown
Contributor Author

@pushrax pushrax Jun 5, 2015

Choose a reason for hiding this comment

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

I had to do this since for some strange reason rb_hash_has_key is a static method...

Copy link
Copy Markdown
Contributor Author

@pushrax pushrax Jun 5, 2015

Choose a reason for hiding this comment

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

There was a patch submitted in 2002 to change this haha. http://blade.nagaokaut.ac.jp/cgi-bin/scat.rb/ruby/ruby-core/86

@dylanahsmith
Copy link
Copy Markdown
Contributor

dylanahsmith commented Jun 5, 2015

LGTM

@fw42
Copy link
Copy Markdown
Contributor

fw42 commented Jun 5, 2015

@csfrancis can you take a look? My Ruby C is weak

@fw42
Copy link
Copy Markdown
Contributor

fw42 commented Jun 5, 2015

For context: This is to make Shopify/liquid#592 work with liquid/c

pushrax
pushrax reviewed Jun 5, 2015
View reviewed changes
Comment thread ext/liquid_c/parser.c
Copy link
Copy Markdown
Contributor Author

@pushrax pushrax Jun 5, 2015

Choose a reason for hiding this comment

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

What's the naming convention for values? I picked v arbitrarily, not sure what's commonly used.

@pushrax
Copy link
Copy Markdown
Contributor Author

pushrax commented Jun 5, 2015

@trishume would be a good review as well, @csfrancis is super-busy

csfrancis
csfrancis reviewed Jun 5, 2015
View reviewed changes
Comment thread ext/liquid_c/parser.c
Copy link
Copy Markdown

@csfrancis csfrancis Jun 5, 2015

Choose a reason for hiding this comment

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

You could maybe be defensive here and assert this is a hash.

@csfrancis
Copy link
Copy Markdown

csfrancis commented Jun 5, 2015

LGTM.

pushrax added a commit that referenced this pull request Jun 6, 2015
@pushrax
Merge pull request #23 from Shopify/use-expression-literal-map
35e9aee 
Use Liquid::Expression::LITERALS to look up parsed literal names
@pushrax pushrax merged commit 35e9aee into master Jun 6, 2015
@pushrax pushrax deleted the use-expression-literal-map branch June 6, 2015 04:57
@fw42
Copy link
Copy Markdown
Contributor

fw42 commented Jun 6, 2015

Should we release a new gem version? Any non-backwards compatible changes since the last release?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@pushrax @dylanahsmith @fw42 @csfrancis