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

Assigned delegate #728

Merged
merged 4 commits into from
Dec 9, 2024
Merged

Conversation

deanebarker
Copy link
Contributor

Allows the execution of a C# delegate whenever values are assigned via an assign or capture tag.

Fluid/TemplateContext.cs Outdated Show resolved Hide resolved
Fluid.Tests/AssignmentCaptureTests.cs Outdated Show resolved Hide resolved
deanebarker and others added 2 commits December 5, 2024 20:39
Co-authored-by: Hisham Bin Ateya <hishamco_2007@yahoo.com>
Co-authored-by: Hisham Bin Ateya <hishamco_2007@yahoo.com>
_parser.TryParse(source, out var template, out var error);
var context = new TemplateContext
{
Assigned = (v) => buffer = v.ToStringValue()
Copy link
Owner

Choose a reason for hiding this comment

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

I'd rather separate both, or there is no way to just extend the assign tag only. If someone wants both then the same event can be added to both.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How do want them named?

CultureInfo = options.CultureInfo;
TimeZone = options.TimeZone;
Captured = options.Captured;
Assigned = options.Assigned;
Copy link
Owner

Choose a reason for hiding this comment

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

Good. It's important we can change the options globally or per context.

/// <summary>
/// Gets or sets the delegate to execute when a member has been assigned via capture or assign
/// </summary>
public Action<FluidValue> Assigned { get; set; }
Copy link
Owner

Choose a reason for hiding this comment

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

Like Captured this should return a ValueTask. But unlike Captured, and that's something that could be changed at some point, we should also provide the TemplateContext as an argument because sometimes you need to take the options into account to do something. I have had to add it in the most recent commit I did, and obsolete the previous methods. So let's not miss it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, should the delegate be async? (warning: async programming is a weak spot for me...)

Copy link
Owner

Choose a reason for hiding this comment

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

What it means is the delegate signature needs to return a ValueTask so instead of Action<FluidValue> it becomes a Func<FluidValue, ValueTask>. Then to add TemplateContext as the input, it becomes Func<FluidValue, TemplateContext, ValueTask>.

I will need to help you though for the invokation since there is an early non-async exit (Awaited method) and I don't expect you to know how to handle that (you can try still).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

...so, I wasn't planning on do anything with the return value. My goal was just to get variables "out" of the template. Do you want something to return and be assigned inside the context?

Copy link
Owner

Choose a reason for hiding this comment

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

Let me update your PR, you'll see. To make something "async" you just need to return "ValueTask" or "Task", you don't use it, the compiler/runtime will.

@sebastienros
Copy link
Owner

PR updated. Also modified the Captured delegate with a breaking change, but makes it more powerful.

@deanebarker
Copy link
Contributor Author

@sebastienros To be totally clear, this is what I'm after. If you want to return the value to the context, that's fine, but goal for this PR was just to get them out.

  [Fact]
  public async Task Assign2()
  {
      var source = @"
          {%- assign a = 'b' %}
          {%- assign c = 'd' %}
      ";

      var dataAssignmentsFromTemplateExecution = new Dictionary<string, object>();

      _parser.TryParse(source, out var template, out var error);
      var context = new TemplateContext
      {
          Assigned = (identifier, value, context) => {
              dataAssignmentsFromTemplateExecution[identifier] = value.ToObjectValue();
              return value;
           }
      };
      var result = await template.RenderAsync(context);
      Assert.Equal("b", dataAssignmentsFromTemplateExecution["a"]);
      Assert.Equal("d", dataAssignmentsFromTemplateExecution["c"]);
      Assert.Equal(2, dataAssignmentsFromTemplateExecution.Count);
  }

@sebastienros
Copy link
Owner

@deanebarker I understand, but since we are there I prefer to make it complete and not have someone ask for the feature late, with another breaking change.

@deanebarker
Copy link
Contributor Author

So my process of just passing the unmodified value back again is conceptually correct for what I want to do?

@sebastienros
Copy link
Owner

Yes, that's the pattern.

@deanebarker
Copy link
Contributor Author

Great. Ship it.

@deanebarker deanebarker closed this Dec 6, 2024
@sebastienros
Copy link
Owner

sebastienros commented Dec 6, 2024

You could write your own extension method like this to have the same pattern as you had initially, while using the overly complicated solution I came up with:

public static TemplateContext OnAssigned(this TemplateContext self, Action<string, FluidValue> assigned))
{
    self.Assigned = (indentifier, value, context) => 
    {
        assigned(identifier, value);
        return ValueTask.FronmResult<FluidValue>(value);
    };
}

@sebastienros sebastienros reopened this Dec 6, 2024
@deanebarker
Copy link
Contributor Author

You mean...complicate it further to avoid complication? ... 😀

@sebastienros
Copy link
Owner

Every programmer should take some philosophy classes.

@deanebarker
Copy link
Contributor Author

"I think, therefore I am... going to over-engineer this for no reason."

/// <param name="value">The value that is assigned.</param>
/// <param name="context">The <see cref="TemplateContext" /> instance used for rendering the template.</param>
/// <returns>The value which should be assigned to the property.</returns>
public delegate ValueTask<FluidValue> AssignedDelegate(string identifier, FluidValue value, TemplateContext context);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure why you split into two delegates and why the signature is the same

Copy link
Contributor Author

@deanebarker deanebarker Dec 7, 2024

Choose a reason for hiding this comment

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

Honestly, I don't quite get this either. Whether you use assign or capture, it's the same basic idea, conceptually.

@sebastienros sebastienros merged commit bd80b7b into sebastienros:main Dec 9, 2024
2 checks passed
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