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

3.1.4 or 3.1.3 breaks custom operators #521

Closed
Big-Whoop opened this issue Jul 8, 2020 · 4 comments · Fixed by #528
Closed

3.1.4 or 3.1.3 breaks custom operators #521

Big-Whoop opened this issue Jul 8, 2020 · 4 comments · Fixed by #528
Milestone

Comments

@Big-Whoop
Copy link

I just updated pebble from 3.1.2 to 3.1.4 and found that my custom operators no longer work. The BinaryOperator Interface has changed while a patch or minor version normally shouldn't introduce breaking changes on public interfaces.

I also found that the documentation hasn't been updated.

I tried to implement the new interface but now all my operators run into a stack overflow with no obvious reason. I Might be doing something wrong or I discovered a bug.

Is it possible to update the doumentation so I can verify if my implementation is correct?

@ebussieres
Copy link
Member

Here's the pull request that has changed the interface

https://github.com/PebbleTemplates/pebble/pull/499/files

@Big-Whoop
Copy link
Author

Hi, thank you, I found my error. I interpreted the "getInstance()" method so that I should return a singleton instance of the Expression instead of creating a new instance each time. If I do the latter it works as expected.

Maybe createInstance() would be a better and less confusing name for this method because getInstance() is a well known method name for a singleton pattern.

ebussieres added a commit that referenced this issue Jul 25, 2020
ebussieres added a commit that referenced this issue Jul 25, 2020
@ebussieres ebussieres linked a pull request Jul 25, 2020 that will close this issue
Merged
@ebussieres
Copy link
Member

I've just adjusted the doc. In another major release, I'll rename the getInstance method, so I'll keep it open

@ebussieres
Copy link
Member

Gonna be available in next release

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 a pull request may close this issue.

2 participants