Possible Coverage of CompanialCop rules?

[MODUSCarstenScholling]

finnpedersenkazes brought this here. Created a new discussion as the original is going to be closed.

Would it make sense to have a look at CompanialCop in this context? It has been two years since their last release.

https://github.com/Companial/CompanialCop

I stopped using it, because I had issues with the installation.
I don't agree with all of their rules, there is also some overlap, but otherwise there might be some good ideas in there.

I have reviewed the rules and some look interesting. What do you think @Arthurvdv ?

Id	Title	Default Severity	Enabled	Remark
CM0001	Name for primary key must be PK.	Warning	Yes	Interesting!
CM0006	IP address must not be present in any part of the source code.	Warning	Yes	Interesting?
CM0014	Position for global variables, triggers and methods must be correct.	Warning	Yes	Interesting!
CM0015	Msg and Err labels should end with a dot, Qst should end with a question mark.	Warning	Yes	Extend AC0014?
CM0017	Object should not have empty sections.	Warning	Yes	Interesting!
CM0022	GridLayout property must not have value Rows	Warning	Yes	Interesting?
CM0031	Object is unused in project.	Warning	No	Interesting!
CM0033	Editable property already exists in object level.	Warning	Yes	Interesting!
CM0035	Global procedure is unused in project.	Warning	No	Interesting?
---	---	---	---	---
CM0004	First 19 field IDs are reserved for primary key fields.	Warning	Yes	-
CM0016	Internal Methods must be invoked with explicit parameters.	Warning	Yes	-
CM0023	The identifier must have at least one of the mandatory affixes.	Warning	Yes	-
---	---	---	---	---
CM0002	Variable with suffix Tok must be locked and locked variables must have suffix Tok.	Warning	Yes	Covered by AC0020, AC0021
CM0003	Procedure name must not contain whitespaces.	Warning	Yes	Covered by Naming Pattern
CM0005	The Enum identifier must be within the allowed range.	Warning	Yes	Covered by AS0099
CM0007	FlowFields must not be editable.	Warning	Yes	Covered by PC0001
CM0008	Commit() must have a comment to justify its existence.	Warning	Yes	Covered by DC0001
CM0009	Use of hardcoded object IDs in functions is not allowed.	Warning	Yes	Covered by LC0003
CM0010	Caption must be specified.	Warning	Yes	Covered by AC0011
CM0011	Procedure prototype must not end with semicolon.	Warning	Yes	Covered by FC0001
CM0012	Procedure must be either local or internal. Public procedures must be documented.	Warning	Yes	Covered by multiple DCxxxx
CM0013	ToolTip must end with a dot.	Warning	Yes	Covered by AC0014
CM0018	Objects need to have the Access/Extensible properties defined.	Warning	Yes	Covered by PC0005, PC0006
CM0019	Local variable name should not contain whitespace/wildcard symbols.	Warning	Yes	Covered by Naming Pattern
CM0020	Global variable name should not contain whitespace/wildcard symbols.	Warning	Yes	Covered by Naming Pattern
CM0021	Parameter name should not contain whitespace/wildcard symbols.	Warning	Yes	Covered by Naming Pattern
CM0024	Empty captions should be locked.	Warning	Yes	Covered by AC0018
CM0025	SetAutoCalcFields must not be invoked on Normal fields.	Warning	Yes	Covered by PC0007
CM0026	Zero (0) Enum Value should be reserved for Empty Value.	Warning	Yes	Covered by AC0019
CM0027	Tables coupled with TransferFields must have matching fields.	Warning	Yes	Covered by PC0020, PC0021
CM0028	When using get the values provided must match the values required by table key.	Warning	Yes	Covered by PC0013
CM0029	Option data type is not allowed.	Warning	Yes	Covered by LC0088
CM0030	Method parameters are not used.	Warning	Yes	Covered by LC0095
CM0032	Property already exists in object level with the same value.	Warning	Yes	Covered by LC0019, LC0020
CM0034	Field assigned in TableRelation is longer than the field being assigned to.	Warning	Yes	Covered by PC0028
CM0036	Event Publishers must be local.	Warning	Yes	Covered by AC0024
CM0038	Open and close parenthesis must be specified after 'IsEmpty' and 'Count'.	Warning	Yes	Covered by FC0003
CM0039	Temporary records should not trigger the table triggers.	Warning	Yes	Covered by PC0027

[someC0d3r]

CM0035: Global procedure is unused in project.

I probably need to look into this in more detail (maybe the CompanionCop considered my points below already 😁), but based on a private project I worked on some time ago, there are a few additional things that may need to be considered when dealing with the usage check for internal procedure:

• Are there any apps listed in internalsVisibleTo?

1. If so, are those apps actually available for analysis, either as a readable .app file or directly through their source code?
2. What should happen if an app listed in internalsVisibleTo is not available?
3. Is the procedure referenced by any of those apps?

• Is the procedure referenced through a string literal, for example via ErrorInfo.AddAction or Notification.AddAction?

CM0006: IP address must not be present in any part of the source code.

Maybe this rule could be extended beyond IP addresses and cover potentially sensitive information in general, such as tokens, secrets, credentials or similar values?

CM0031: Object is unused in project.

Exceptions here could be: Objects for job queue, API pages?
But generally I also like the rule itself.

---

I'm with you, they have a few rules which quite interesting. :)

[finnpedersenkazes]

CM0001: Name for primary key must be PK.

I disagree. Code readability is the number one requirement.

My logic

There are different variants (or kinds) of tables. Depending on the variant there are different rules.

Variants

• Setup Table: field(1; "Primary Key"; Code[10])
• Lookup Table: field(1; Code; Code[20])
• Cue Table: field(1; "Primary Key"; Code[10])
• Normal Table: field(1; "Entry No."; Integer)

I describe these patterns in instructions for the AI and then I ask for a code review.

Thanks @MODUSCarstenScholling for a good analysis.

[someC0d3r]

I think by "primary key", they don't mean the names of the fields that make up the primary key, but rather the name of the key itself, e.g.:

fields
{
    <Fields here>
}
keys
{
    key(PK, <Primary Key Fields...>) { }
}

If that's not the case, then I don't think the rule would be particularly useful either, because there are many perfectly valid cases where the fields forming the primary key need specific, descriptive names, for example:

1. Document Type, Document No., Line No.
2. Customer No., Ship-to Code
3. Entry No. (as you already mentioned under Variants)
4. ...

So in my opinion, enforcing a naming convention for the primary key definition itself makes probably sense, but enforcing one for the fields that happen to form the primary key would not (like you've already said)