jeremiah-c-leary / vhdl-style-guide

Style guide enforcement for VHDL
GNU General Public License v3.0
177 stars 38 forks source link

Reducing duplication of rule identifiers in the codebase #1117

Closed JHertz5 closed 5 months ago

JHertz5 commented 5 months ago

What is your question? I have noticed that when a rule is constructed, there is a bit of duplication occurring in the __init__(). Take, for example the code for case_001:

from vsg.rules import token_indent

from vsg import token

lTokens = []
lTokens.append(token.case_statement.case_keyword)
lTokens.append(token.case_statement.end_keyword)
lTokens.append(token.case_statement_alternative.when_keyword)

class rule_001(token_indent):

    def __init__(self):
        token_indent.__init__(self, 'case', '001', lTokens)

The rule identifier, '001' is in the name of the class, and is hard-coded into the init method. We could use the special variables that are built in to Python classes to extract the '001' string from the name of the class and remove the duplication, and thereby reduce the work, the risk of mistakes, etc that comes with duplication.

Is this change desirable? I'll produce a PR with proposed changes, more than happy for it to be thrown away if the current method is preferred.


It could be possible to attempt a similar thing with the name attribute of the rule as well, since usually the name matches the name of the module that the subclass is in, e.g. the signal_XXX rules have the name "signal" and are in a module called "signal". However, this is not always the case, e.g. the rules with the name "assert" are in a module called "assert_statement", so there would have to be a bit of renaming/refactoring if it was desirable to put this in place.

JHertz5 commented 5 months ago

I have created a draft PR, #1118 to demonstrate the change. I will update the PR when I have time, likely this weekend, to remove the now-redundant identifier input to the Rule.__init__() method.

jeremiah-c-leary commented 5 months ago

Morning @JHertz5 ,

I really like this idea. I can't count the number of times I have not updated either name or identifier, which is why there is always a test to check them. However, ensuring they are correct by using code is a much better solution.

For the name attribute, we could check for exceptions and replace. For example, the if rules are in the if_statement directory.

dir_name = get_current_directory_name()
if dir_name == 'if_statement':
    rule_name = 'if'
else:
    rule_name = dirname

That could be a stop gap until a real refactoring of rule names could take place.

Or maybe the directory names can change to match the rule name? For some reason I named the rule directory if_statement instead of if. I would like to think there was some conflict with python since if is a python keyword. but then again maybe not.

--Jeremy

JHertz5 commented 5 months ago

Hi @jeremiah-c-leary.

Amazing, I'm glad that you like the suggestion! I'll be glad if I can make your life a tiny bit easier.

For the name attribute, we could check for exceptions and replace. For example, the if rules are in the if_statement directory.

That is a great idea. I'll include this in the PR, and renaming of rules or directories can come later. You might find it preferable to have infrastructure for exceptions; maybe there are cases where you'd like to have a more detailed directory name while keeping the rule name short, or as you say, there might be some conflict/confusion with Python keywords.

JHertz5 commented 5 months ago

Hi @jeremiah-c-leary, thanks again for your advice. I have marked PR #1118 as "ready for review". There are a lot of files touched, which means that there is a high likelihood of causing conflicts on other branches, but they should be easy to resolve. Apart from the changes in vsg/rule.py and in the tests directory, the changes are just removing the name and identifier arguments from init functions everywhere.

jeremiah-c-leary commented 5 months ago

Afternoon @JHertz5 ,

I had to change the match to an if statement in the get_rule_name function to support pre 3.10 python. Otherwise it looks really good. It sure cleans up with the code by reducing the number of arguments passed to the base rule class.

There were almost a 1000 files changed, so I appreciate the effort you put in to make development easier. I will have to keep my eye open for any other similar improvements that can be made.

I will push this to master.

Thanks again,

--Jeremy