Add option for custom error messages

[PelleEikeberg]

Is your feature request related to a problem? Please describe.

I made a VSG yaml ruleset for following some coding conventions I had, and saw that you did not have an option for snake_case. This was no issue as I could easily make a regex for solving this. For note: the regex under will not catch single letter prefixes or single letter suffixes, as I have separate rules for these (e.g instantiation_008: case:lower instantiation_601: prefixes: [i_].

rule:
    group:
        case:
            disable: false
            phase: 6
            case: regex
            regex: (?![a-z||0-9]\u005F)(?!.*\u005F[a-z||0-9]$)([a-z||0-9]+(?:\u005F[a-z||0-9]+)*)
            suffix_exceptions: [_n,_a,_s,_s1,_s2,_s3,_s4,_s5,_d,_d1,_d2,_d3,_d4,_d5,_p,_p1,_p2,_p3,_p4,_p5,_dp,_dn]
            case_exceptions: [IEEE]
            error_msg: "Group name must be in snake_case"

Describe the solution you'd like

My request is not about the snake_case (although it would be nice), but rather that there is an optional field for custom error message So that I can choose what error is displayed when the rule fails. (exemplified by the "error_msg:" option under). And that this field can be set in the json/yaml file.

Describe alternatives you've considered

I guess I could do something like this using Python and/or change the VSG code, but it would be nice to have it as part of the config file.

Additional context

While running through my new rules, I noticed that the rules: library_500 and use_clause_500 gives the error:

----------------------------+------------+------------+--------------------------------------
  Rule                      |  severity  |  line(s)   | Solution
----------------------------+------------+------------+--------------------------------------
  library_500               | Error      |          1 | Change "ieee" to "IEEE"
  use_clause_500            | Error      |          2 | Change "ieee" to "IEEE"
  use_clause_500            | Error      |          3 | Change "ieee" to "IEEE"

This is not according to the group rules I've set, but they are enabled because of the group (i disabled all rules in global). Why they break when they pass the regex is beyond me, but I feel like the error format should be the same as the rest of the error messages set by the regex: Format **** to match defined regex.

If you are to implement the error_msg then obviously I want them to follow the same hierarchy rule and be superseded by the lower lever rule (global message is removed and the group is applied if both as present)

[PelleEikeberg]

If you want to add snake_case then this regex will give you those: (([a-z||0-9]+(?:\u005F[a-z||0-9]+)*)), or: ((?!.*[A-Z])([\w]+(?:\u005F[\w]+)*)). They do match part of the words but I assume those cases give error in your ruleset.

[JHertz5]

Hi @PelleEikeberg

Thanks for raising this issue. There are a couple of things to address here.

Snake Case

I'm glad that you found a regex that works for you to implement snake_case, but out of curiosity - what is it about the lowercase setting that does not satisfy your needs? Valid VHDL names/identifiers can only contain digits, letters or underscores, so if your letters are all lowercase, then snake_case (lowercase letters, separated by underscores) is covered (at least in my opinion). I use snake case for naming in all of my projects and I've had my needs met by setting the rules to lowercase. I'd be interested to know if there is something that you feel is missing that we can incorporate into a snake_case setting for you.

IEEE Error

You mentioned that you were seeing errors telling you to change ieee to IEEE, and you're not sure why these errors are being raised. Even though ieee matches your regex, the config snippet that you posted includes the following line:

            case_exceptions: [IEEE]

This line of your config is telling the VSG to ignore the regex for the string IEEE and only accept the given case (i.e. all uppercase). That is why ieee raises an error. If you delete that line in your config, you should see that the errors no longer appear.

Custom Error Messages

I'm afraid that I don't think that it is a good idea to open up error messages to user configuration.

Each rule should have a solution message that is relevant, clear and helpful. We are all using the same set of rules, so the solution message for each rule should be relevant to everyone's use case. If the solution message is not relevant, clear and helpful enough, then, ideally, the user should raise an issue here, so that VSG can be improved and everyone who uses it can benefit.

The main concern that I have is that users could misunderstand the rule and rewrite the solution message to be less helpful. In an extreme case, a user could accidentally rewrite the message for library_500, which is about a string being uppercase or lowercase, to be "Correct the indentation of this line." Their team, who is using the configuration they've written, are thinking "my indentation is already correct, why is this stupid tool not working properly?", when they actually need to correct something else entirely to get rid of the error.

It also has the potential to end up being a lot of work. Currently there are solution messages that are dynamically changed to be more helpful, e.g. Change "ieee" to "IEEE" includes the string that is causing the error and provides the change needed to correct this error. This is very helpful; because of the case_exception that you've set in your config, if the solution message just said Group name must be in snake_case, you might be tearing your hair out, saying "The group name is already in snake case! Why is this raising an error?! There's a bug in the tool!", but this dynamic solution message helped us to very easily find the cause of the errors.

So users might want to be able to make their own dynamic solution messages. We then need to define a syntax that can be used to specify the original string, the corrected string, the number of indents, etc. And then we'd need to validate all of those different possible custom solution messages ("What does the tool do if you try to include the number of indents in a solution message for a rule that is about the case?").

In summary, if it were up to me, I would say that we shouldn't go ahead with this feature request.

[jeremiah-c-leary]

Morning @PelleEikeberg and @JHertz5 ,

YAML configuration

I think you might want to change your YAML file to be this:

rule:
    group:
        case:
            case::name:
                disable: false
                phase: 6
                case: regex
                regex: (?![a-z||0-9]\u005F)(?!.*\u005F[a-z||0-9]$)([a-z||0-9]+(?:\u005F[a-z||0-9]+)*)
                suffix_exceptions: [_n,_a,_s,_s1,_s2,_s3,_s4,_s5,_d,_d1,_d2,_d3,_d4,_d5,_p,_p1,_p2,_p3,_p4,_p5,_dp,_dn]
                case_exceptions: [IEEE]
                error_msg: "Group name must be in snake_case"

This would ensure the regex does not apply to keywords, which do not have any underscores in it. VSG defaults all case to lowercase.

Snake Case

There is a discussion in issue #1202 on changing the definition of camelCase and PascalCase which might be of interest.

What is your goal with the snake_case regex? Are you looking to force snake_case?

I ask because I put your regex examples into regex101.com and put in some example strings:

A string without an underscore still passes the regex.

As @JHertz5 mentioned, identifiers are limited to numbers, letters, and underscores as defined by this production:

basic_identifier ::= letter { [ underline ] letter_or_digit }

letter_or_digit ::= letter | digit

I think @JHertz5 is correct that just using lower case would meet your case needs. Unless you want to force at least one underscore, but then it appears you may need to change your regex.

Custom Error Messages

@JHertz5 makes a compelling argument to not allow users to change the error message. Could you elaborate on your use case? There may be a better alternative to meet your need.

Regards,

--Jeremy

[PelleEikeberg]

Thanks so much, @JHertz5 for a thorough explanation!

Snake Case:

You make a compelling point of using only lower (default) instead of my custom "snake_case". I will, however, keep my regex for the reason of my custom convention only allowing for some prefix/suffix endings in all names, therefore having the negative capture group: (?!.*\u005F[a-z||0-9]$) and (?![a-z||0-9]\u005F), only allowing my pre-set list of suffix_exceptions and prefix_exceptions. I understand however why there is no need to add snake_case to the VSG settings as it is covered by lower

IEEE Error:

Thank you so much for explaining how it works! That was helpful :)

Custom Error Messages:

I understand your explanation for not using the existing message. However, in the case of regex I would like an option for explaining why a regex was put in place. Could appending a message be a solution? I would much like to know why a rule was set in place in my config .yaml file. An example of appending a custom message might be:

================================================================================
Phase 1 of 7... Reporting
Total Rules Checked: 194
Total Violations:    1
  Error   :     1
  Warning :     0
----------------------------+------------+------------+--------------------------------------
  Rule                      |  severity  |  line(s)   | Solution
----------------------------+------------+------------+--------------------------------------
  signal_004                | Error      |         24 | Format ShouldFail to match defined regex [config_msg: "UPPERCASE and single letter prefix/suffix not allowed"]

As the default could be nothing, this would only add functionality and not restrict it. I would also make it clear what is the VSG message, and the user custom config message, thus not "blaming" VSG for confusing messages.

[jeremiah-c-leary]

Evening @PelleEikeberg ,

Custom Error Messages

I understand your explanation for not using the existing message. However, in the case of regex I would like an option for explaining why a regex was put in place.

Ah. That makes sense. Since the regex is supplied by the user, then only the user understands what is required to pass the regex.

Could appending a message be a solution?

That seems like a reasonable solution. The only other solution I can see would be to report it at the end of the output, but then the user must bounce back and forth between lines to understand what to do.

Maybe we use custom_error_message instead of config_msg, or maybe 'additional_information`? Maybe add in the author of the message?

rule:
    group:
          case::name:
              disable: false
              case: regex
              regex: (?![a-z||0-9]\u005F)(?!.*\u005F[a-z||0-9]$)([a-z||0-9]+(?:\u005F[a-z||0-9]+)*)
              suffix_exceptions: [_n,_a,_s,_s1,_s2,_s3,_s4,_s5,_d,_d1,_d2,_d3,_d4,_d5,_p,_p1,_p2,_p3,_p4,_p5,_dp,_dn]
              case_exceptions: [IEEE]
              custom_error_message_author:  PelleEikeberg
              custom_error_message: "UPPERCASE and single letter prefix/suffix not allowed"

================================================================================
Phase 1 of 7... Reporting
Total Rules Checked: 194
Total Violations:    1
  Error   :     1
  Warning :     0
----------------------------+------------+------------+--------------------------------------
  Rule                      |  severity  |  line(s)   | Solution
----------------------------+------------+------------+--------------------------------------
  signal_004                | Error      |         24 | Format ShouldFail to match defined regex [PelleEikeberg: "UPPERCASE and single letter prefix/suffix not allowed"]

Hmm...maybe just the custom_error_message?

rule:
    group:
        case:
            case::name:
                disable: false
                case: regex
                regex: (?![a-z||0-9]\u005F)(?!.*\u005F[a-z||0-9]$)([a-z||0-9]+(?:\u005F[a-z||0-9]+)*)
                suffix_exceptions: [_n,_a,_s,_s1,_s2,_s3,_s4,_s5,_d,_d1,_d2,_d3,_d4,_d5,_p,_p1,_p2,_p3,_p4,_p5,_dp,_dn]
                case_exceptions: [IEEE]
                custom_error_message: "PelleEikeberg:  UPPERCASE and single letter prefix/suffix not allowed"

I believe appending a custom error message would be the correct solution. Just struggling with how to complex to make the implementation.

How about user_error_message?

rule:
    group:
        case:
            case::name:
                disable: false
                case: regex
                regex: (?![a-z||0-9]\u005F)(?!.*\u005F[a-z||0-9]$)([a-z||0-9]+(?:\u005F[a-z||0-9]+)*)
                suffix_exceptions: [_n,_a,_s,_s1,_s2,_s3,_s4,_s5,_d,_d1,_d2,_d3,_d4,_d5,_p,_p1,_p2,_p3,_p4,_p5,_dp,_dn]
                case_exceptions: [IEEE]
                user_error_message: "UPPERCASE and single letter prefix/suffix not allowed"

================================================================================
Phase 1 of 7... Reporting
Total Rules Checked: 194
Total Violations:    1
  Error   :     1
  Warning :     0
----------------------------+------------+------------+--------------------------------------
  Rule                      |  severity  |  line(s)   | Solution
----------------------------+------------+------------+--------------------------------------
  signal_004                | Error      |         24 | Format ShouldFail to match defined regex [user_error_message: "UPPERCASE and single letter prefix/suffix not allowed"]

I think I like your idea to put the message in brackets. It helps to indicate to the user that this is something unique.

What do think?

Regards,

--Jeremy

[PelleEikeberg]

This would server my purpose well. I agree with having user-defined messages in brackets to separate the message. I like the naming user_error_message, because even though it is a long format name, it will only show up if a user actually uses the field.

I can see about making a pull request with this change, but I am no Python expert and thought it was better to just come with the suggestion.

[jeremiah-c-leary]

Morning @PelleEikeberg ,

I can see about making a pull request with this change, but I am no Python expert and thought it was better to just come with the suggestion.

I can implement the solution for this. If you would like to contribute there are still a number of rules that could be implemented. If you are interested, just let me know, I can guide you through the process.

Regards,

--Jeremy

[jeremiah-c-leary]

Afternoon @PelleEikeberg ,

I pushed an update for this issue to the issue-1358 branch. When you get a chance could you check it out on your end and let me know what you think?

Thanks,

--Jeremy

[jeremiah-c-leary]

Morning @PelleEikeberg ,

Just wanted to ping you on this issue to see if you had a chance to check it out.

Thanks,

--Jeremy

[PelleEikeberg]

Looks good to me! However I have not run an exhaustive test, so there might be some issues still. If you push this into main I would be quite happy. I Will however keep using this branch for the time being! Thank you so much!

NOTE: As far as I am concerned, you can close the issue

[jeremiah-c-leary]

Evening @PelleEikeberg ,

Looks good to me!

Awesome.

However I have not run an exhaustive test, so there might be some issues still.

I will keep this issue open until February 3rd. If I do not hear anything from you I will assume everything is okay and merge to master.

Regards,

--Jeremy