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

Reserve keyword arguments for new options in assert_template_result #1612

Merged
merged 3 commits into from
Sep 1, 2022

Conversation

dylanahsmith
Copy link
Contributor

Problem

As mentioned in #1611

The test/integration folder is meant to be a set of tests that can be shared with other liquid implementations, initially used with liquid-c. However, it still has coupling to the liquid library API, which could be further reduced.

There were tests that couldn't be converted with the current assert_template_result & assert_match_syntax_error interface, since they require additional options to be supported. For instance, there is a need to specify the file system, error mode and to render liquid errors. Once that is done, more integration tests can be decoupled from the library API.

However, we can't just add keyword arguments to assert_template_result without calls to it that pass assigns as keyword arguments.

Solution

Update calls to assert_template_result that pass assigns as keyword arguments and instead pass a hash explicitly.

I also turned the message positional argument to assert_template_result into a keyword argument, which also reserves the ability to add additional keyword arguments without breaking its callers.

I also removed an unused assert_template_result_matches method and removed an unnecessary assigns argument to assert_match_syntax_error

@dylanahsmith dylanahsmith force-pushed the assert-template-result-reserve-kwargs branch from 2beb930 to 0787660 Compare September 1, 2022 21:38
@dylanahsmith dylanahsmith merged commit 433ed0f into master Sep 1, 2022
@dylanahsmith dylanahsmith deleted the assert-template-result-reserve-kwargs branch September 1, 2022 21:39
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