Additional check for LC0040?

[jwikman]

I like the idea of LC0040, and think it is great to be explicit on all new code.

But activating LC0040 on an existing (old) app will probably result in a lot of warnings for a lot of us. One way is to just replace them all with false as RunTriggerparameter, to ensure that you do not change anything that is not broken. But then you've added to the code that you've explicitly do not want the run the trigger code, without really knowing if this is by intention or someone just overlooked it to begin with.

Going through all LC0040 manually and make a decision one-by-one is time consuming.

Hence, I suggest an additional check to LC0040, to only be fired when needed:

Only trigger LC0040 If there are code in the OnInsert/OnModify/OnDelete trigger.

In that way I only need to go through the statement that really adds value. It would also result in LC0040 showing up if you later on adds code into the trigger.

Thoughts?

[Arthurvdv]

What if my App dependents on your App where I want to add customer logic to the OnInsert trigger?

codeunit 50100 MyCodeunit
{
    [EventSubscriber(ObjectType::Table, Database::"My Custom Table PTE", OnAfterInsertEvent, '', false, false)]
    local procedure OnAfterInsertEventOnMyCustomTable(var Rec: Record "My Custom Table PTE"; RunTrigger: Boolean)
    begin
        if RunTrigger then
            exit;

        // My Business Logic here
    end;
}

Not raising the rule could give a unexpected result in this scenario if I'm not mistaken?

I would opt for adding pragma's (with a TODO) to review these in a future moment in time.

[jwikman]

I don't get it, but maybe I'm just slow... OnAfterInsertEvent events in dependent apps that relies on the RunTrigger parameter will always be affected if I change the RunTrigger parameter when I call Insert(), but does this rule really help for that as it is now? I cannot know about any dependent app and their logic anyway.

I think that the rule can only do the exception for tables in the current app. For tables in other apps, I think that it is good as it is. But in the apps I work with I use 95% custom tables, and there it would be nice to only get this warning on tables that has a trigger.

I just saw a scenario in a PR today, where there were a Insert() (without parameter) since many years. Some time ago, someone added a OnInsert trigger (This was probably before we could use "Find all references" on the OnInsert trigger). LC0040 was not enabled yet, since it was hundreds of warnings... If we had enabled LC0040 and added a pragma around Insert() that, we wouldn't be helped in this case. But if we had enabled LC0040 and we only got the warning on tables that has the trigger implemented, we would not need to add pragmas - AND we would have got the warning at the time someone added the OnInsert trigger (and forgot to use "Find All References").

That was the reason behind my suggestion, and I still believe that it would make more sense than to add hundreds of pragmas. (But I also appreciate that it might be too much work to implement to be worth the effort...)

[Arthurvdv]

Ah, I understand better the additional check you're looking for now. Thank you for taking the time to elaborate on this.

I'll try to see if this is possible to implement this. Not sure at this moment, but I'll give it a try!

[jwikman]

Thanks @Arthurvdv 👍

[pri-kise]

Don't forget, that you can have define Triggers in TableExtensions. They are only raised when the RunTrigger is true. How would we deal with this scenario?

[tinestaric]

I get the idea behind the suggestion. But when we were adding the missing RunTrigger parameters, our default was true, as realistically, there are only a handful of scenarios when you want to skip it (TempTables, ChangeCompany...).

I'd be wary of adding the condition that it's raised only for tables with triggers defined, because, as mentioned, we should assume, that every table can have triggers extended by a 3rd party. At best, if something like this was added, it should be optionally turned on, otherwise, we'll still end up with many triggers not being run for extending apps.

[jwikman]

I get your points. The code that got me starting on this issue is quite old, and a lot of the code is converted from C/AL. And back in the C/AL times I know that a lot of devs (me included, it seems... 😬) skipped the RunTrigger (probably from laziness) if there were no code in the trigger - it was, after all, our own code that we handled by 100% without dependencies. That is why we now sit with a lot of Insert() and Modify() without any parameter...

So, either this might be a new rule - or maybe we should just scrap this idea totally?

[Arthurvdv]

@jwikman I'm not going to be able to add extra logic to this rule. I don't see a way of a consistent determine the presents of these triggers (and if the triggers indeed contain code or are "empty" triggers).

On top of that cases like TableExtensions, EventSubscribers, etc could give a false sense of safety when the rule isn't raised.

there are only a handful of scenarios when you want to skip it (TempTables, ChangeCompany...).

This! In most cases you want to have set the RunTrigger set to True, where the current implementation of the rule corresponds to this.

If a new rule could be created as a variant of this one, create a new discussion here: https://github.com/StefanMaron/BusinessCentral.LinterCop/discussions.

[jwikman]

No worries @Arthurvdv, it's better to focus on rules that makes new code better than rules that makes it easier to handle existing code. :)