In/NotIn and const rules show enum values for enum type

[elb3k]

Problem

The error message for enum in/not_in rules show numbers instead of enum values
Sample case

Input:

...
enum Enum {
    UNKNOWN_ENUM = 0;
    OPTION_1 = 1;
    OPTION_2 = 2;
}

message Book {
    Enum enum1 = 1 [(validate.rules).enum = {in: [1, 2]}];
    Enum enum2 = 2 [(validate.rules).enum = {not_in: [1]}];
    Enum enum3 = 3 [(validate.rules).enum = {const: 1}];
}

Current output:

func (m *Book) validate(all bool) error {
	...
	if _, ok := _Book_Enum1_InLookup[m.GetEnum1()]; !ok {
		err := BookValidationError{
			field:  "Enum1",
			reason: "value must be in list [1 2]",
		}
		...
	}

	if _, ok := _Book_Enum2_NotInLookup[m.GetEnum2()]; ok {
		err := BookValidationError{
			field:  "Enum2",
			reason: "value must not be in list [1]",
		}
		...
	}
        if m.GetEnum3() != 1 {
		err := BookValidationError{
			field:  "Enum3",
			reason: "value must equal 1",
		}
		...
	}
}

Desired output:

func (m *Book) validate(all bool) error {
	...
	if _, ok := _Book_Enum1_InLookup[m.GetEnum1()]; !ok {
		err := BookValidationError{
			field:  "Enum1",
			reason: "value must be in list [OPTION_1 OPTION_2]",
		}
		...
	}

	if _, ok := _Book_Enum2_NotInLookup[m.GetEnum2()]; ok {
		err := BookValidationError{
			field:  "Enum2",
			reason: "value must not be in list [OPTION_1]",
		}
                ...
	}
        if m.GetEnum3() != 1 {
		err := BookValidationError{
			field:  "Enum3",
			reason: "value must equal OPTION_1",
		}
		....
	}
}

Changes

• Added isEnum function to check if the field type is enum
• Added enumList function to return options string (e.g. [OPTION_1 OPTION_2] ) for given field and in/notIn list
• Added enumVal function to return option (for const)
• Change cpp, goshared templates

TODO

• Add test cases
• Update compatibility set

[CLAassistant]

All committers have signed the CLA.

[elliotmjackson]

Hi @elb3k! welcome to PGV and thank you for raising this. I know this is still a draft... just want to connect with you on a few things:

1. Could you help out your reviewers with a detailed description of what you're trying to do and what problem you're trying to solve?
2. Running CI locally is your best way to make sure everything is passing... view the development section of our readme for more detail on how to do this - your feedback on this section is welcomed and encouraged.
3. Lastly, adding tests to cover your changes and updating the compatibility set is always a good way to improve your review outcome.

Good luck - reach out if you need anything.

[elb3k]

Hi @elb3k! welcome to PGV and thank you for raising this. I know this is still a draft... just want to connect with you on a few things:

1. Could you help out your reviewers with a detailed description of what you're trying to do and what problem you're trying to solve?
2. Running CI locally is your best way to make sure everything is passing... view the development section of our readme for more detail on how to do this - your feedback on this section is welcomed and encouraged.
3. Lastly, adding tests to cover your changes and updating the compatibility set is always a good way to improve your review outcome.

Good luck - reach out if you need anything.

Thanks, @elliotmjackson will address all comments!

[elb3k]

Hey @elliotmjackson.
I added support for golang and cpp (my primary usecases)
and java seems to have this already (in/not_in uses Arrays.toString() and const uses Enum_Val.toString())

But can't wrap my head around adding python support.
Or using python example. Can you give some pointers?

[elliotmjackson]

relates to #638 (comment)

[elliotmjackson]

I sympathise with you on the test harness @elb3k, I dont totally understand it myself... from a high-level, tests are defined in the protos found in tests/harness/cases, the resulting code is generated with make testcases and the harness can be run with make ci. Probably start with tests/harness/cases/enums.proto and define a message which utilises your changes, then head on over to tests/harness/executor & enumCases to add your case assertions.

[elb3k]

Contributor

Awesome! If python supports it by default, will add test cases and test harnesses

[elb3k]

@elliotmjackson added the support for python too. Current harness test cases cover all of the cases with error messages.
But the test cases do not match the exact messages. Making a change to exactly match the error message would be significant uplift from me.

[elb3k]

Appreciate it @elliotmjackson!