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

Python Booleans and Filter base with parsing #523

Conversation

michaelpro1
Copy link
Contributor

@michaelpro1 michaelpro1 commented Oct 30, 2020

This change includes the following

  • Support of python style booleans for portability of templates - True and False are added into ExtendedParser
  • AbstractFilter that can be used for simpler filters which works by introspecting Jinjavadoc.params section with some automatic conversions

@boulter
Copy link
Contributor

boulter commented Oct 30, 2020

Thanks for the contribution. Could you break out the YAML filters into their own PR?

Copy link
Contributor

@boulter boulter left a comment

Choose a reason for hiding this comment

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

Thanks!

@michaelpro1
Copy link
Contributor Author

michaelpro1 commented Oct 31, 2020

Take a look at this, I'll open PR for YAML after this
FYI prettier-java errors out on my machine with posix error, could you do that once you are happy with the code?

@michaelpro1 michaelpro1 changed the title YAML Filters, Python Booleans and Filter base with parsing Python Booleans and Filter base with parsing Oct 31, 2020
Copy link
Contributor

@boulter boulter left a comment

Choose a reason for hiding this comment

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

Looks pretty good. I have a few small comments.

It is a pretty low-level change though so we will have to be careful about rolling this out.

import java.util.Map;
import org.junit.Test;

public class AbstractFilterTest extends BaseInterpretingTest {
Copy link
Contributor

Choose a reason for hiding this comment

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

nice tests, thank you.

@michaelpro1
Copy link
Contributor Author

Looks pretty good. I have a few small comments.

It is a pretty low-level change though so we will have to be careful about rolling this out.

Absolutely, if you have a perf test to compare before/after that would be good

@boulter
Copy link
Contributor

boulter commented Nov 5, 2020

Absolutely, if you have a perf test to compare before/after that would be good

I'm less concerned about performance than compatibility with existing templates and unknowns. We can see how it goes.

@boulter
Copy link
Contributor

boulter commented Nov 5, 2020

FYI prettier-java errors out on my machine with posix error, could you do that once you are happy with the code?

Prettier reported no changes for me.

Copy link
Contributor

@boulter boulter left a comment

Choose a reason for hiding this comment

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

just a few more nits. Thanks again.

@michaelpro1
Copy link
Contributor Author

Prettier reported no changes for me.

Doesn't seem to work on Windows but OK with WSL. NodeJS executable is an odd dependency.

I'm less concerned about performance than compatibility with existing templates and unknowns. We can see how it goes.

I've implemented the .filter(String[]) method as well which should behave as before and how most filters retrieve their params.

As this change is only applied to 2 filters, the rest can be modified when needed. e.g. Filters with single param will work without an issue, however where >1 params are involved further testing is warranted.

@boulter
Copy link
Contributor

boulter commented Nov 9, 2020

@mattcoley any concerns?

Copy link
Collaborator

@mattcoley mattcoley left a comment

Choose a reason for hiding this comment

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

🆒 LGTM

@boulter boulter merged commit 8de83d1 into HubSpot:master Nov 9, 2020
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.

3 participants