feat: support additional args as mapping source #22
Conversation
|
Thanks for your PR! I appreciate your contribution to the project. I'll need some time to go through the changes in detail, so I'll get back to you in a few days. In the meantime, I may provide small suggestions or fixes while reviewing. These will be minor improvements to align with the project's guidelines. Let me know if you have any questions! |
pkg/builder/assignment.go
Outdated
| return | ||
| } | ||
| index-- | ||
| if int(index) >= len(additionalArgs) || index < 0 { |
There was a problem hiding this comment.
| if int(index) >= len(additionalArgs) || index < 0 { | |
| if index < 0 || len(additionalArgs) <= int(index) { |
This makes it easier for the human brain to visualize the valid range.
c.f. https://llewellynfalco.blogspot.com/2016/02/dont-use-greater-than-sign-in.html
| return nil, logger.Errorf("%v: src type is not defined. make sure to be imported", p.fset.Position(src.Pos())) | ||
| } | ||
| if util.IsInvalidType(src.Type()) { | ||
| if util.IsInvalidType(dst.Type()) { |
pkg/builder/postprocess.go
Outdated
| } | ||
|
|
||
| func ordinalNumber(n int) string { | ||
| if n >= 11 && n <= 13 { |
There was a problem hiding this comment.
| if n >= 11 && n <= 13 { | |
| if 11 <= n && n <= 13 { |
There was a problem hiding this comment.
interesting to have this function😄
Note: We won't have numbers 100 or more here, so it's okay with the current implementation.
pkg/parser/comment.go
Outdated
| return logger.Errorf(`%v: to use ":reverse", style must be ":style arg"`, p.fset.Position(posReverse)) | ||
| } | ||
|
|
||
| if opts.Reverse && len(opts.TemplatedNameMapper) > 0 { |
There was a problem hiding this comment.
| if opts.Reverse && len(opts.TemplatedNameMapper) > 0 { | |
| if opts.Reverse && 0 < len(opts.TemplatedNameMapper) { |
pkg/builder/postprocess.go
Outdated
| return nil, logger.Errorf("%v: manipulator function %v 2nd arg type mismatch", p.fset.Position(m.Pos), ret.FuncName()) | ||
| } | ||
|
|
||
| if len(m.AdditionalArgs) > 0 { |
There was a problem hiding this comment.
| if len(m.AdditionalArgs) > 0 { | |
| if 0 < len(m.AdditionalArgs) { |
pkg/builder/method.go
Outdated
| // reverse cannot be used with additional arguments | ||
| if m.Opts.Reverse { | ||
| additionalArgs = nil | ||
| } |
There was a problem hiding this comment.
@youta1119
I was wondering why additionalArgVars is omitted when :reverse is used.
Would it be possible to support both together? Could you share the reasoning behind this decision? Is there a logical constraint that prevents it, or is it just difficult to implement?
There was a problem hiding this comment.
To support additional arguments with :reverse, I believe it is necessary to extend the converter definition syntax to allow defining multiple return values, as shown in the example below.
However, this idea is difficult to implement due to the need for syntax extension.
type Convergen interface {
// :recv r
// :reverse
// :style arg
ReverseWithAdditionalArg(
dst *Pet,
) (src *model.Pet, arg1 int, arg2 string)
}As an alternative, I also considered an idea that does not require syntax extension, as in the example below.
However, this idea is complicated because the definition of input arguments is split between the function's arguments and return values.
type Convergen interface {
// :recv r
// :reverse
// :style arg
ReverseWithAdditionalArg(
dst *Pet,
arg1 int, // well be assigned to $2
arg2 string, // well be assigned to $3
) (src *model.Pet) // well be assigned to $1
}I think the former idea is better.
However, it is difficult to implement, so I decided not to implement it in this pull request.
There was a problem hiding this comment.
I prefer the latter.
But anyway, I'm going to the current PR first, and then I'd like to see if any requests come up regarding this.
Again, thanks for your contribution!
youta1119
force-pushed
the
feature/support-additional-args
branch
from
fd9fa7b to
8431ba8
Compare
|
@youta1119 Great job ❤️ @reedom When can we expect new release with this change? |
|
Oh, I've forgotten to put a new tag. |
3 participants
Fixes #17
To extend the :map notation, it is now possible to assign template values such as
$1,$2, and$3to the mapping sources.Template values start with
$1, and additional arguments are assigned to$2and beyond(However, for compatibility reasons, the first argument$1is optional).Also, preprocess and postprocess now support additional arguments.