Rule suggestions from recent and future additions to Companial Cop

[tinestaric]

Hey,

about a year ago I did a similar post where I described a couple of rules that we have implemented internally that I think could be interesting for the wider community. I think it's time for another batch :D

I'll list them down, and for the interesting ones I can later create a pull request

1. When using Get, the values provided must match the values required by the table key
   A rule to catch.Get() with too few, or wrong parameters. It ignores Get with 0 parameters which are usually used for setups.

2. Option data type is not allowed
   Self-explanatory. Everything should be an enum.

3. DataClassification and ApplicationArea Property already exists on the object level
   Checks for properties on the object and field level, if a duplicate property is found, a warning is thrown.

4. Redundant Editable property
   Checks for Editable property defined on a field, when Editable property has been defined on the object or the base object in case of Extension objects. If the base object is not editable, the properties on fields are redundant.

5. The field assigned in the TableRelation is longer than the field being assigned to
   This one is to prevent runtime issues where a field of length 10, has a table relation to a field of length 20 for example.

6. Event publishers must be local
   Keeping publishers local gives you the benefit of freely extending the parameters with additional fields without causing a breaking change.

7. IsEmpty and Count should have parenthesis specified
   For some reason, these two functions are not covered by the default warning and we wanted to have consistency

8. Temporary records should not trigger the table triggers
   In most cases, running the triggers on the Temp table will cause issues, as the code might read/insert/modify/delete data that's no longer temporary.

9. (Future) Throw a warning for redundant using directives
   Not yet implemented. Check for "using" directives that are not needed in the object.

10. (Future) Check for "This"
    Not yet implemented. The default rule AA0248 checks that both global variables and local procedures are prefixed with "this". We find it useful on global variables, but not on local procedures, so we want to implement the same rule, but split the rule IDs, so we can silence one, but keep the other.

11. (Future) Enums with 0 value need to have InitValue on fields
    Not yet implemented. If Enum does not have a 0 value, is used on a field, and doesn't have InitValue, then when Initializing a record, you don't have valid data in the field, unless later specify it.

[jwikman]

These all look very good!
Just a small comment on 6, also allow internal event publishers. That is often used when you have events in a Facade Codeunit that is called from the Implementation Codeunit.

[Arthurvdv]

It feels like it's almost Christmas, because these would be great gifts! 🫡

1. Yes
2. Maybe
3. Yes
4. Yes
5. Yes
6. Yes (with the remarks of @jwikman)
7. Yes
8. Yes
9. Yes
10. I don't have enough experience to assess this rule (pun intended 😁)
11. Yes

Option data type is not allowed (2)
While I absolutely agree on the "everything should be an enum", I'm wondering if this could have the side affect that ExampleA becomes ExampleB (instead of applying a pragma).
A nice-to-have could be if the method you're accessing requires a option as param, to not raise the rule in that case?

procedure ExampleA()
var
    ReservationMgt: Codeunit "Reservation Management";
    Mode: Option "None","Allow deletion",Match;
begin
    ReservationMgt.SetReservSource(Rec);
    ReservationMgt.SetItemTrackingHandling(Mode::"Allow deletion");
    ReservationMgt.DeleteReservEntries(true, 0);
end;

procedure ExampleB()
var
    ReservationMgt: Codeunit "Reservation Management";
begin
    ReservationMgt.SetReservSource(Rec);
    ReservationMgt.SetItemTrackingHandling(1);
    ReservationMgt.DeleteReservEntries(true, 0);
end;

[tinestaric]

Maybe we'll get them all in by Christmas :DDD

I can get working on preparing the PRs. What's the easiest way for you to handle them? Last time I did about three rules per PR.

Good point regarding point 2. In our case, that was an exception for which we'd use pragma.
The proposed nice-to-have could work, but I would not like it if a field is defined as an option and then used in a procedure that has an option parameter. Fields in my opinion should never be options anymore.

[tinestaric]

Another note, number 9 is now part of the MS warnings, but hidden by default:
https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/diagnostics/diagnostic-al792

[fvet]

1. When using Get, the values provided must match the values required by the table key

Please don't forget that one can get a record using it's recordid as well.
SalesLine.Get(SalesLineRecordId) is in fact a valid code statement.

[rvanbekkum]

Test cases:

    local procedure TestRecordGetLocalVariable()
    var
        ItemVariant: Record "Item Variant";
    begin
        // Warning
        ItemVariant.Get('10000');
    end;

    local procedure TestRecordGetLocalVariableNoWarning()
    var
        ItemVariant: Record "Item Variant";
    begin
        // NO warning
        ItemVariant.Get('10000', 'VARIANTCODE');
    end;

    local procedure TestRecordGetParameter(var ItemVariant: Record "Item Variant")
    begin
        // Warning
        ItemVariant.Get('10000');
    end;

    local procedure TestRecordGetReturnValue() ItemVariant: Record "Item Variant"
    begin
        // Warning
        ItemVariant.Get('10000');
    end;

    local procedure TestRecordGetMethod()
    begin
        // Warning
        TestRecordGetReturnValue().Get('10000');
    end;
    
    local procedure TestRecordGetReportDataItem()
    begin
        // Warning
        InvLineDataItem.Get('10000');
    end;

    local procedure TestRecordGetXmlPortTableElement()
    begin
        // Warning
        InvLineXmlPortTableElement.Get('10000');
    end;

    local procedure TestRecordGetSetupTableNoArgumentsProvided()
    var
        CompanyInformation: Record "Company Information";
    begin
        // NO warning
        CompanyInformation.Get();
    end;

    local procedure TestRecordGetSetupTableCorrectArgumentsProvided()
    var
        CompanyInformation: Record "Company Information";
    begin
        // NO warning
        CompanyInformation.Get('');
    end;

    local procedure TestRecordGetSetupTableIncorrectArgumentsProvided()
    var
        CompanyInformation: Record "Company Information";
    begin
        // Warning
        CompanyInformation.Get('', 12345);
    end;

    local procedure TestRecordGetLocalVariableRecordID()
    var
        ItemVariant: Record "Item Variant";
        MyRecordId: RecordId;
    begin
        // NO warning
        ItemVariant.Get(MyRecordId);
    end;

    local procedure TestRecordGetParameterRecordID(MyRecordId: RecordId)
    var
        ItemVariant: Record "Item Variant";
    begin
        // NO warning
        ItemVariant.Get(MyRecordId);
    end;

    local procedure TestRecordGetReturnValueRecordID() MyRecordId: RecordId
    var
        ItemVariant: Record "Item Variant";
    begin
        // NO warning
        ItemVariant.Get(MyRecordId);
    end;

    local procedure TestRecordGetMethodRecordID()
    var
        ItemVariant: Record "Item Variant";
    begin
        // NO warning
        ItemVariant.Get(TestRecordGetReturnValueRecordID());
    end;

    local procedure TestRecordGetBuiltInMethodRecordID()
    var
        ItemVariant: Record "Item Variant";
    begin
        // NO warning
        ItemVariant.Get(ItemVariant.RecordId());
    end;

    local procedure TestRecordGetFieldRecordID()
    var
        ApprovalEntry: Record "Approval Entry";
    begin
        // NO warning
        ApprovalEntry.Get(ApprovalEntry."Record ID to Approve");
    end;