New Rule0088: Avoid option types

[iwanscheidsnc]

Description
Option types should be avoided, use enum if applicable.

Reason for this rule
Option types should be avoided to get rid of some of the bad patterns that come along with option fields.

Bad code example:

Good code example:

[Arthurvdv]

Thank you for providing this PR.

I hardest challenge for this rule is to prevent false positives, we're (unnecessary) pragma's then needs to be applied. With some fine-tuning, like the check for IsEventSubscriber you've applied (great one!) will help to avoid this.

Some remarks;

• The diagnostic shows always (Option), in the message. Does this needs to be the name of the variable?
• Add the ctx.IsObsoletePendingOrRemoved() check to exclude obsolete objects (fields/tables)
• Can we handle the "Option dependency" example, where I believe it's valid choice for an Option type. An unwanted side effect could be that the code is changed to an integer, like ReservationManagement.SetItemTrackingHandling(1);, where I believe the code is then less readable.
• Should we exclude tables with specific TableType property like CDS? The AL Table Proxy Generator converts an OptionSet from Dataverse to an Option type to Business Central. See the statecode field in the "Example CDS table" below.
• Would be great if you could include automated tests

If I can assist on something, feel free to let me know.

Example Option dependency

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

Example CDS table

table 50100 "Dataverse Project PTE"
{
    ExternalName = 'prefix_project';
    TableType = CDS;
    Description = '';
    Caption = 'Project';

    fields
    {
        field(1; prefix_projectId; Guid)
        {
            ExternalName = 'prefix_projectid';
            ExternalType = 'Uniqueidentifier';
            ExternalAccess = Insert;
            Description = 'Unique identifier for entity instances';
            Caption = 'Project';
        }
        field(2; prefix_name; Text[100])
        {
            ExternalName = 'prefix_name';
            ExternalType = 'String';
            Description = 'The name of the custom entity.';
            Caption = 'Name';
        }
        field(25; statecode; Option)
        {
            ExternalName = 'statecode';
            ExternalType = 'State';
            ExternalAccess = Modify;
            Description = 'Status of the Project';
            Caption = 'Status';
            InitValue = " ";
            OptionMembers = " ",Active,Inactive;
            OptionOrdinalValues = -1, 0, 1;
        }
    }
}

[Arthurvdv]

I have a proof-of-concept to suppress the diagnostic if the variable is used as a parameter for a method which is outside of the extension itself. Let me know if I can assist on this and work further on this.

[iwanscheidsnc]

Thanks for the fast reply.

1. "The diagnostic shows always (Option), in the message." It should list the options like this
   . Maybe its not always working.
2. I added IsObsoletePendingOrRemoved
3. Tables with type CDS should now be ignored
4. I added some tests
5. I changed Warning to Info

It would be great if you could help to suppress the diagnostic if the variable is used as a parameter for a method which is outside of the extension itself and handle the deppendeny example.

[Arthurvdv]

@iwanscheidsnc, Great work on the changes! I've did some work on dependency example which I've added to this PR.

We're almost there I believe. I only have one last question on the diagnostic that shows always (Option) in the message.

Can you share how a warning shows in VS Code on a table with a field op type Option?

[iwanscheidsnc]

Thanks for the help.

Yes, for option fields it always shows "Option types (Option) should be avoided, use enum if applicable".

For variables it shows the name of the variable

Then for option fields the name of the field should be displayed (currently not the case). But i don't know how much that would help to unterstand the info message. I think we could leave out the parameter and always just say "Option types should be avoided, use enum if applicable"?

[Arthurvdv]

... just say "Option types should be avoided, use enum if applicable"

I would vote for this! No need to make it more complex then necessary :-)

[Arthurvdv]

Let's go ahead and merge this into the Pre-Release to gather feedback on this new rule.

Again thank you for contributing this new rule to the LinterCop!