Events handled parameter must be var

[f4n0]

Happens a lot, even Microsoft has made a mistake like that.
if the Event signature, contains a parameter called "Handled" must be passed by var

[Arthurvdv]

..or IsHandled. But do we really want to hardcode this based on the name?

There are more cases, like for example a temporary table should also passed by var.

Just thinking at loud here; maybe an other way around is all event publisher with “OnBefore” in the name should pass all variables as var?

[f4n0]

well, a rule like:
If is an event, if the parameter is Boolean, if the parameter name contains "handled" then 99% need to be var

[TKapitan]

I agree that there are more cases where it's needed, but in my case, I usually discover such a problem during the code review. But I did not put much effort into the IsHandled checks.

Maybe we can start with IsHandled (I think the name is now standardized), and if it's needed, it can be extended later to some other use cases (especially as IsHandled will be easy to implement, temporary records or temporary tables probably not so easy).

[Arthurvdv]

@f4n0 and @TKapitan , thank you for the idea and proving feedback here!

I've created a new rule: LC0073 - Handled parameters in event signatures should be passed by var which is now available in the v0.31.3 (pre)release version of the LinterCop.

If is an event, if the parameter is Boolean, if the parameter name contains "handled" then 99% need to be var

This made me confided to set the default severity to Warning instead of a Info. Hopefully we're not breaking to much pipelines out there when this go's from the Pre-Release into the Latest release of the LinterCop.

[TKapitan]

Thanks @Arthurvdv. Would it be much effort to not show the warning for obsoleted events? I don't have an issue adding pragma, but I know that we have a few projects where we still have the events as obsoleted and not removed (events we had to obsolete to fix the IsHandled)

[Arthurvdv]

The warning should not trigger if the method itself has an [Obsolete] decorator. Additionally, if the object, such as a codeunit, has the property ObsoleteState = Pending; set, the warning should also not trigger.

I’ve added two test scenarios (#813), and without making any changes, these tests passed successfully.

Am I missing something here, or do you perhaps have an example for me?

[Arthurvdv]

@TKapitan, did you have the chance to look into this?

[Arthurvdv]

@TKapitan?

[TKapitan]

@Arthurvdv all seems OK.

[Arthurvdv]

Thank you for confirming this, then I can close the PR with a clear conscience 🤗