vektra / mockery

A mock code autogenerator for Go
https://vektra.github.io/mockery/
BSD 3-Clause "New" or "Revised" License
6.04k stars 406 forks source link

Templating functions changes (Follow up to #639) #673

Open jippi opened 1 year ago

jippi commented 1 year ago

Hey,

Following up from #639 where I submitted template function support to the config file.

I came back to this feature today in another repository, and I ended up wasting a solid 2h debugging why

dir: 'internal/mocks/{{ .InterfaceDirRelative | trimPrefix "internal/" }}'

didn't work as expected., turns out that it had to be

dir: 'internal/mocks/{{ trimPrefix .InterfaceDirRelative "internal/" }}' (ordering of the args) instead...

Which was not what my muscle memory from tools like consul-template trimPrefix.

I wanted to submit this ticket to discuss if we should change the behavior to work like "normal" templating filters like "most" of the Go and Python eco-system do or leave it "as is".

I'm happy to submit the change if you think it's work doing :)

LandonTClipp commented 1 year ago

Is this happening because the filter is adding the value as the second argument to trimPrefix? It seems to be the case: https://pkg.go.dev/text/template#hdr-Pipelines

Unfortunately as already being part of the public API, I'm not sure we could simply tell mockery to "swap" the arguments if that breaks the bare function call like {{ trimPrefix .InterfaceDirRelative "internal/" }}. I was really worried about what consul is doing and unfortunately my fears were justified: https://github.com/hashicorp/consul-template/blob/74fa6ffdcec51ade85a8a2008254868d5b670447/template/funcs.go#L1035

That's sad, I didn't have the foresight to see this before the PR was merged. I'm not sure what a good non-breaking solution would be. I suppose we could hide the "correct" behavior behind a feature flag 🤷🏻

jippi commented 1 year ago

I wonder if we can make it compatible by sniffing number of arguments (or an empty 2nd arg) in trimPrefix and adjust the behavior / raise an error from there - with a thin wrapper around it - could even make it a migration path to detect empty 2nd arg and emit warning and deprecate the old? 🤔

LandonTClipp commented 1 year ago

I might just mark this as something to be fixed in v3, I don't like getting into argument sniffing trickery because it just gets convoluted. If you really want, we could hide behind a feature flag and deprecate not having the feature flag turned on? Ugh. On the other hand, how useful is the filter syntax really going to be to people at large? As long as we document that it's not supported, is it really going to be an issue for people?

jippi commented 1 year ago

Works for me - I don't love it either - but the frustration today was real, mostly because it was entirely self-inflected at both consumer and producer side of the problem - no one else to blame 😆

I'm fine with a v3 thing (also happy to do the work at that time), but I think it would be fine either way. I just love when things "looks and feels" the same across my tooling - the mental overhead (clearly, in my case) is real when it doesn't :)

Might just be my personal bias - I'm honestly fine either way - just wanted to flag this to you for thoughts :)

LandonTClipp commented 1 year ago

I'll think about it, if you really really want to, I'd be fine with accepting a feature flag and defaulting to use that in v3, since it is indeed how most other filtering things work. Otherwise I think just documenting it would be sufficient for most people.

LandonTClipp commented 1 year ago

@jalaziz I see, so you probably need something like this (theoretical example)?

dir: "{{ split .InterfaceDirRelative '/' | index 0 }}/mocks/{{ split .InterfaceDirRelative '/' | slice 1 | join '/' }}"

Actually, the only thing in that template that wouldn't work is the join. The other functions I believe would work since those are globally defined by text/template.

I can see the need for it, I'm truly okay with adding a flag like template_pipeline: true that will swap the args and just noting it in the docs.

jalaziz commented 1 year ago

I ended up getting it to work with:

{{ index (splitAfterN .InterfaceDirRelative "/" 2) 0 }}/mocks/{{ index (splitAfterN .InterfaceDirRelative "/" 2) 1 }}'

It's not pretty, but it works. Defining it as a pipeline would be slightly more readable.

Actually, the only thing in that template that wouldn't work is the join. The other functions I believe would work since those are globally defined by text/template.

As far as I can tell, index isn't defined to be pipeline friendly :(.