WIP: Show a better error on duplicate command#38
Conversation
|
Hey found the bug and have a PR up here: Feel free to give it a quick once over. Thanks! -TW |
|
Okay, so with the line number fixed... I'm actually not sure if there's anything left to do here, I did do this in a bit of a rush, but now I look at it again, I'm not sure there's much missing. Maybe some tests - but I'm not sure what the right place for those would be? Anything you'd like changed? |
|
I've updated go.mod to use the latest version of the |
| startLine := -1 | ||
| var t token.Token | ||
| var name string |
There was a problem hiding this comment.
Are we gaining anything with the init to -1 ( vs var startLine int ) ?
For symmetry, I would likely arrange them as:
var name string
var startLine int
var t token.Token
With t being short-lived, have it assigned closer to the block its used in.
Have name and startLine defined in the order they generally appear (assignment and return)
Lastly, line may be a sufficient name
There was a problem hiding this comment.
(sorry, I haven't had a chance to look at updating this yet)
Are we gaining anything with the init to -1 ( vs var startLine int ) ?
The main gain in my mind is that -1 is obviously invalid, whereas 0 is potentially correct (logic depending :))
But sure, I can change it to be a 0 default.
There was a problem hiding this comment.
The main gain in my mind is that -1 is obviously invalid
I get that as a concept and generally agree, but, because the value is aways assigned from t.Line(), is there any way for that default value to get returned?
That's why I didn't think we were gaining anything ...
|
I'm going to merge this. The changes that I want are small and I can make them in post. Thanks for this PR - I suspect that having the cmd line number handy will prove even more useful in the future ! -TW |
|
Thanks, sorry for not being able to follow up. I've had a busy week. |
Usecase:
Gives:
run: Duplicate command: foo defined on line 4 -- originally defined on line 0Gives:
run: Duplicate command: version defined on line 0 -- this command is built-inMarked as WIP because the first line is showing up with line 0. I'm not sure if this is a bug in the parser/tokenizer, or if I'm missing something. It seems strange that the second definition is correctly showing up as line 4...