WIP: Show a better error on duplicate command

[rburchell]

Usecase:

foo:
    echo "hello"

foo:
    echo "world"

Gives: run: Duplicate command: foo defined on line 4 -- originally defined on line 0

version:
    echo "hello"

Gives: run: Duplicate command: version defined on line 0 -- this command is built-in

Marked 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...

[TekWizely]

Hey found the bug and have a PR up here:

• fix: Line 1 showing as 0 go-parsing#9

Feel free to give it a quick once over.

Thanks!

-TW

[rburchell]

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?

[TekWizely]

I've updated go.mod to use the latest version of the go-parsing libs which includes the fix for the line0 bug.
Please re-base your branch and see if it helps !

[TekWizely]

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

[rburchell]

(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.

[TekWizely]

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 ...

[TekWizely]

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

[rburchell]

Thanks, sorry for not being able to follow up. I've had a busy week.