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

Refactor API #149

Merged
merged 14 commits into from Dec 17, 2017
Merged

Refactor API #149

merged 14 commits into from Dec 17, 2017

Conversation

epage
Copy link
Member

@epage epage commented Dec 16, 2017

This is a major step towards #95

  • ParserBuilder -> Parser
    • Takes in includes, tags, blocks, and filters
  • Parser -> Template
    • Takes in str
  • Template -> String
    • Takes in globals
  • Separate concerns
    • Top-level Parser, Template
    • Value
    • Interpreter
    • Syntax / Compile to interpreted state

This also saw a consolidation of how variables are processed between pure-dots (name.value) and indexing (name["value"]). Not complete and indexing isn't yet available everywhere, but it should be easier to make it so.

This consolidation included error reporting. Now non-existent variables will always cause an error rather than being treated as false / "".

The other major format-breaking change is I removed for_loop variable which is a duplicate of forloop, the latter being shopify compliant.

This also saw a huge performance change for rendering

Before:

test bench_parse_template  ... bench:      24,825 ns/iter (+/- 1,831)
test bench_parse_text      ... bench:       3,702 ns/iter (+/- 255)
test bench_parse_variable  ... bench:       5,294 ns/iter (+/- 447)
test bench_render_template ... bench:      38,182 ns/iter (+/- 3,325)
test bench_render_text     ... bench:       8,858 ns/iter (+/- 541)
test bench_render_variable ... bench:      10,018 ns/iter (+/- 503)

After

test bench_parse_template  ... bench:      23,337 ns/iter (+/- 2,152)
test bench_parse_text      ... bench:       2,662 ns/iter (+/- 191)
test bench_parse_variable  ... bench:       4,544 ns/iter (+/- 306)
test bench_render_template ... bench:       8,996 ns/iter (+/- 482)
test bench_render_text     ... bench:       2,231 ns/iter (+/- 133)
test bench_render_variable ... bench:       7,188 ns/iter (+/- 412)

This is an intermediate step.  The two APIs are still intermixed but
this should be enough to start making the amount of progress on cobalt-org#95
needed for some cobalt changes that are blocked on liquid.
This is currently a one-time API until filters are made copyable.

BREAKING CHANGE: Original API exists under `liquid::syntax` and does not
automatically add any filters, tags, or blocks.
While at it, I put in an optimization for Filters, ParseTags, and
ParseBlocks to reduce heap allocations when using `fn`s.

BREAKING CHANGE: `Filter` has changed from a `Fn` to a trait.
Like truthiness, indexing logic is now consolidated.  This also made it
easier to consolidate the IdentifierPath and Variable logic.

This does cause some inconsistency with shopify/liquid that we'll need
to consider.  They allow undefined variable access.

BREAKING CHANGES:
- `{ non_existent }` will now error rather than
  nothing.  This particularly impacts `{ non_existent | default "fallback" }`.
  - Improves consistency between non-indexed and indexed behavior
  - Increase error reporting
- value has now moved from `syntax::Value` to `value::Value`.
`for` loops provided both a `forloop` and `for_loop` variable, only the
former being shopify compatible.

BREAKING CHANGE: Change `for_loop` to `forloop`.
@epage epage changed the title Refactor APIO Refactor API Dec 16, 2017
The CI failed saying it couldn't find the file
`\\\\C:\\\\projects\\\\liquid-rust\\\\tests/fixtures/input/example.txt`

My best guess is that `.join` on Windows isn't splitting path components
on `/`, so I'm doing it explicitly.
@epage epage merged commit 6d601a2 into cobalt-org:master Dec 17, 2017
@epage epage deleted the api branch December 17, 2017 04:23
@epage epage mentioned this pull request Dec 17, 2017
16 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant