Produce error for duplicate object names - #50
Conversation
hazelwiss
left a comment
hazelwiss
left a comment
There was a problem hiding this comment.
left some minor notes.
| match ty { | ||
| ItemType::Value => { | ||
| if values.values.contains_key(&ident) { | ||
| emitter.emit(Rich::custom( |
There was a problem hiding this comment.
So if emitter is emitted from then, what, it will result in an error? Seems like it I think but it is not super clear.
| } | ||
|
|
||
| #[test] | ||
| #[should_panic] |
There was a problem hiding this comment.
I am not sure how we feel about should panic tests where we do not more clearly specify the output of the panic, given well it could potentially panic for unrelated reasons? I suppose we hope that those panics are caught by other tests.
There was a problem hiding this comment.
I think it's better to have them than to not have them at least.
There was a problem hiding this comment.
Yep, we need to move the error messages into the panic message to be able to test the specific output. Alternatively, we could try to have tests that don't panic on errors but instead check them for certain content. We could open an issue for this.
Merge #50 to the correct branch
3 participants
Can be merged after #49
Previously the value from the last defined object was used. Now we produce an error.