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

add feature flags to jinjava config #1066

Merged
merged 5 commits into from
May 3, 2023

Conversation

boulter
Copy link
Contributor

@boulter boulter commented May 2, 2023

When building #1065, I didn't like adding yet another property to the JinjavaConfig builder. I figured we could use a simple Feature Flags implementation.

Partially inspired by https://ff4j.org/ and https://www.togglz.org/ .

This can replace any boolean configs like enablePreciseDivideFilter, enableRecursiveMacroCalls, failOnUnknownTokens, nestedInterpretationEnabled and more in the future.

@boulter boulter changed the title add features flags to jinjava config add feature flags to jinjava config May 2, 2023
Copy link
Contributor

@jasmith-hs jasmith-hs left a comment

Choose a reason for hiding this comment

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

This is definitely more sustainable than the current approach, and it lets you define features for extensions of Jinjava!


import java.util.function.Supplier;

public class DelegatingFeatureActivationStrategy implements FeatureActivationStrategy {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think DisabledFeatureActivationStrategy, DelegatingFeatureActivationStrategy, and StaticFeatureActivationStrategy are redundant implementations as you can use a lambda:

FeatureConfig
  .newBuilder()
  .add(name1, () -> false)
  .add(name2, () -> true)
  .add(name3, supplier)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. I wanted some simple and standard way to get strategies that were always or off. I removed those classes and added some static delegating strategies instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

The DelegatingFeatureActivationStrategy boilerplate can be removed too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done and also isActive now gets Context objects so it can make decisions based on other data that's in there, perhaps a user or customer userid.

Comment on lines 4 to 9
public static final FeatureActivationStrategy INACTIVE = DelegatingFeatureActivationStrategy.of(
() -> false
);
public static final FeatureActivationStrategy ACTIVE = DelegatingFeatureActivationStrategy.of(
() -> true
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public static final FeatureActivationStrategy INACTIVE = DelegatingFeatureActivationStrategy.of(
() -> false
);
public static final FeatureActivationStrategy ACTIVE = DelegatingFeatureActivationStrategy.of(
() -> true
);
public static final FeatureActivationStrategy INACTIVE = () -> false;
public static final FeatureActivationStrategy ACTIVE = () -> true;


import java.util.function.Supplier;

public class DelegatingFeatureActivationStrategy implements FeatureActivationStrategy {
Copy link
Contributor

Choose a reason for hiding this comment

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

The DelegatingFeatureActivationStrategy boilerplate can be removed too

@boulter boulter merged commit a3841a9 into master May 3, 2023
@boulter boulter deleted the jboulter-add-features-flags-to-jinjava-config branch May 3, 2023 13:22
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