refactor: Finalize message handling refactoring #299
Conversation
matz3
commented
Sep 12, 2024
matz3
commented
- Use new addMessage signature in all reporters
- Reduce duplicate code for addMessage by moving logic into LinterContext
- Only produce actual messages at the end of linting (Might slightly reduce peak memory usage)
- Use new addMessage signature in all reporters - Reduce duplicate code for addMessage by moving logic into LinterContext - Only produce actual messages at the end of linting (Might slightly reduce peak memory usage)
matz3
force-pushed
the
refactor-add-message
branch
from
ebadeb5 to
e5d3f32
Compare
There was a problem hiding this comment.
Looks like some solid improvements 👍
Maybe @d3xter666 wants to have a look too, otherwise fine with me.
| addMessage<M extends MESSAGE>( | ||
| id: M, argsOrNode?: MessageArgs[M] | SaxTag, node?: SaxTag | ||
| ) { | ||
| if (!argsOrNode) { |
There was a problem hiding this comment.
This validation seems redundant and a bit conunterintuitive- the argument is optional, but we do a manual check for it. It also seems to break the definitions above as the second argument there is mandatory.
Why don't we rely entirely on TS validation here?
There was a problem hiding this comment.
To me this all seems required to distinguish between the signatures at runtime. Feel free to improve this part. I couldn't manage to make it better. The only alternative would be to not have optional arguments at all, but then it was hard for me to come up with a type for the messages without args.
| line: 78, | ||
| message: 'Use of deprecated property \'sap.ui5/models/odata-v4-via-dataSource/settings/synchronizationMode\' of sap.ui.model.odata.v4.ODataModel', |
There was a problem hiding this comment.
It seems that the path to the dataSource got lost and only the property is left.
Was this intentional?
I'm fine with it as the positiinalInfo (line/column) is correct
There was a problem hiding this comment.
Good catch. The message was better before.
