Fix #49 and #36 - use go scanner for parsing lines and identifiers #50
Conversation
|
@pdrum Can you review this please? It looks good to me. |
parse/parse.go
Outdated
|
|
||
| // Does the heavy lifting of taking a line of our code and | ||
| // sbustituting a type into there for our generic type | ||
| func subTypeIntoLine(l, t, specificType string) string { |
There was a problem hiding this comment.
I think maybe having line and type rather than l and t would make things a little more readable.
There was a problem hiding this comment.
Yeah, line(string) was another function in this file so I've renamed that to makeLine(string). type is a go keyword so I've hastily settled on the less-concise typeTemplate.
parse/parse.go
Outdated
| // the start or ends before the end of the literal | ||
| if i > 0 || i < len(lit)-len(t) { | ||
| // replace the literal with a capitalized version | ||
| cap := wordify(specificType, unicode.IsUpper(rune(lit[0]))) |
There was a problem hiding this comment.
For the comment preceding it to be true, shouldn't the parameter always be true? For example for someNumberType with NumberType=int I guess we want to have someInt. Doesn't it output someint as is? (I didn't run it, just asking)
parse/parse.go
Outdated
| @@ -40,6 +40,67 @@ var unwantedLinePrefixes = [][]byte{ | |||
| []byte("//go:generate genny "), | |||
| } | |||
|
|
|||
| func subIntoLiteral(lit, t, specificType string) string { | |||
| var i int | |||
| var newLine string | |||
There was a problem hiding this comment.
I guess now that you have extracted this piece into a function, this variable shouldn't be called newLine anymore. Maybe newLiteral or sth.
parse/parse.go
Outdated
| func subIntoLiteral(lit, t, specificType string) string { | ||
| var i int | ||
| var newLine string | ||
| for { |
There was a problem hiding this comment.
I think now that this function is just replacing the occurrences in a single literal, this for loop can be avoided or simplified. Maybe just replacing the first occurrence with an if and then replacing all other occurrences with a single call to Replace.
There was a problem hiding this comment.
Totally right. Nice! I think this refactoring addresses your other issues/questions with this function as well.
|
@justnoise Thank you so much for the changes. It looks great. I will merge this along with #51 on Thursday or Friday to address issues tagged with this milestone. Before that I will add some tests in another PR and perform some manual checks to make sure everything works and all those issues are resolved. I opened #51 a while ago but it's not reviewed yet. Would you consider reviewing it if you felt like it maybe? Also I was wondering if you could take the time to squash the two last commits, so that the last one pertinent to renaming of imports won't appear in git history. |
justnoise
force-pushed
the
use-go-scanner
branch
from
575115c
to
2de26d8
Compare
|
Good call on squashing the last commit. Thanks for the good review and pushing all this through. Due to the lack of activity in this repo, I didn't expect to see these changes merged (I made the change so I could use genny in one of my projects...) so I'm really happy with how this turned out. |
I ran into some issues of genny finding identifier boundaries. It seems that others have run into these issues as well:
The parser was updated to use go's golang scanner (go/scanner) to find identifier boundaries instead of splitting on whitespace and checking for characters at the alphanumeric boundaries.