Lower/upper case rules.

[m-kru]

As an example take rule constant_004 (This rule checks the constant name is lower case). Now imagine that I want the opposite, the constant name must be upper case. There is no such rule. There are 2 possible options:

1. I can add new rule constant_015 (This rule checks the constant name is upper case), but to enable this rule I need to set 2 options in config file to maintain backward compatibility (rule constant_004 is enabled by default):

   # Disable constants to be lower case
   constant_004:
       disable: true
   # Force constants to be upper case
   constant_015:
       disable: false

   One solution is to have such rules disabled by default, then to configure the case, user would have to set only one option in config file (no overhead). However this breaks backward compatibility (I do not know if you care about this), as rule constant_004 is enabled by default.

2. Add case parameter to the rule constant_004. Then only one option would need to be set in config file, and it would maintain backward compatibility (lower case by default):

   # Force constants to be upper case
   constant_004:
       case: upper

What do you think? Which solution do you prefer? Or maybe you have better idea?

[jeremiah-c-leary]

Hey m-kru,

I think the second option would be preferred. It keeps backwards compatibility and gives the user the option to change the case to their preference. I could also keep the configuration file smaller.

Let's say you wanted everything uppercased except the process keyword (process_004) and the architecture keyword (architecture_004). I believe the following configuration would work:

---
rule:
  global:
    case: upper
  process_004:
    case: lower
  architecture_004:
    case: lower
...

As VSG is configuring the rules, if an attribute does not exist it should just ignore it. This would allow users to minimize the number of rules to configure.

There are probably a handful of rules that would need to be updated in vsg/rules:

lower_case_rule.py lowercase_word_after_colon_rule.py lowercase_word_rule.py uppercase_end_label.py uppercase_word_rule.py

These could be updated with the case attribute and default to the appropriate case. I think this would cover all the case checking rules to handle both lower and upper case. We could probably refactor lowercase_word_rule.py and uppercase_word_rule.py into a single rule.

I really like this idea.

Thank you.

[m-kru]

Are you going to implement this, or do you want me to try?

[jeremiah-c-leary]

I think it would be awesome if you tried it. If you have any questions just let me know.

[m-kru]

Can you explain the conceptual difference between lower_case_rule and lowercase_word_rule, and why you sometimes use lower case, but the other time lowercase.

[jeremiah-c-leary]

It looks like lower_case_word_rule is used in the following rules: type_004 variable_004 while lower_case_rule is used for everything else.

lowercase_word_rule takes the line, splits it and passes the a single word to check.is_lowercase. lower_case_rule takes a line, splits it and passes each item in the list to check.is_lowercase.

hmmm...this is interesting. Comparing signal_004 to variable_004, I used utils.extract_signal_names to get the signal name and then pass it to check.is_lowercase.

I checked the log on variable_004 and it has the helpful comments of "Refactoring". It was probably an attempt to resolve some duplication in code that CodeClimate detected.

So there are a couple of possibilities to handle this:

1) Keep lowercase_word_rule separate Pro: Provides special case for lower casing words? Con: slightly confusing 2) Refactor variable_004 and type_004 to rules like signal_004 Pro: less confusing There are extract functions in utils.py that return the type and variable names. Con: Similar code in multiple places. 3) Refactor rules like signal_004 to match variable_004 Pro: Less similar code in multiple places. Con: for signal...there can be multiple signals on a single line.

I am stuck on which would be the better option. What is your opinion?

As for lower_case versus lowercase, it was just inconsistency on my part. For some reason I struggled with this, I have no idea why.

[m-kru]

Ok, it looks like there are more problems with the code, if vhdl file conatains Constant keyword I get:

Traceback (most recent call last):
  File "/home/mkru/.local/bin/vsg", line 11, in <module>
    load_entry_point('vsg==0.41', 'console_scripts', 'vsg')()
  File "/home/mkru/.local/lib/python3.7/site-packages/vsg-0.41-py3.7.egg/vsg/__main__.py", line 177, in main
    oRules.report_violations(commandLineArguments.output_format)
  File "/home/mkru/.local/lib/python3.7/site-packages/vsg-0.41-py3.7.egg/vsg/rule_list.py", line 177, in report_violations
    iFailures += oRule.report_violations(iLineNumber, sOutputFormat, self.oVhdlFile.filename)
  File "/home/mkru/.local/lib/python3.7/site-packages/vsg-0.41-py3.7.egg/vsg/rule.py", line 50, in report_violations
    sOutputString += self._get_solution(iLineNumber)
TypeError: can only concatenate str (not "NoneType") to str

[m-kru]

Ok, here is what I would do. I would replace all *case*_rule.py, in vsg/rules directory, with single case_rule.py containing case parameter. Then in specific rules I would use extract_ functions to extract words the rule concerns.

[jeremiah-c-leary]

That is a great idea. It will reduce the number of rules under vsg/rules.

[m-kru]

How to run regression tests locally, in the repo, when I make some changes I want to be sure that I do not break anything.

[jeremiah-c-leary]

Use this command at the top level: python -m unittest discover

I will add and issue to document it.

[m-kru]

4 tests fail. It would be good to fix them before case rules refactoring.

[jeremiah-c-leary]

which tests?

[m-kru]

FAIL: test_reverse_multiple_configuration_w_rule_disable (vsg.tests.vsg.test_vsg.testVsg)

FAIL: test_reverse_yaml_multiple_configuration_w_rule_disable vsg.tests.vsg.test_vsg.testVsg)

FAIL: test_single_configuration_w_rule_disable (vsg.tests.vsg.test_vsg.testVsg)

FAIL: test_single_yaml_configuration_w_rule_disable (vsg.tests.vsg.test_vsg.testVsg)

[jeremiah-c-leary]

hmm...that is interesting. They are not failing locally for me or on Travis-CI. Could there be something different between our environments?

[m-kru]

Hmm ok, I use arch (rolling release), I will investigate the issue.

[m-kru]

All 4 fails are similar so lets analyze one of them test_reverse_multiple_configuration_w_rule_disable.

Here is the exact results:

FAIL: test_reverse_multiple_configuration_w_rule_disable (vsg.tests.vsg.test_vsg.testVsg)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/mkru/workspace/vhdl-style-guide/vsg/tests/vsg/test_vsg.py", line 60, in test_reverse_multiple_configuration_w_rule_disable
    self.assertEqual(lActual, lExpected)
AssertionError: Lists differ: ['entity entity', 'entity entity1', 'entity[67 chars], ''] != ['']

First differing element 0:
'entity entity'
''

First list contains 8 additional elements.
First extra element 1:
'entity entity1'

- ['entity entity',
-  'entity entity1',
-  'entity is',
-  'end end',
-  'end entity',
-  'end entity1',
-  'port port',
-  'port (',
-  '']
? ^

+ ['']
? ^

And here is the code of this test function:

    def test_reverse_multiple_configuration_w_rule_disable(self):
        lExpected = []
        lExpected.append('')

        lActual = subprocess.check_output(['bin/vsg','--configuration','vsg/tests/vsg/config_4.json','vsg/tests/vsg/config_3.json','--output_format','syntastic','-f','vsg/tests/vsg/entity1.vhd'])
        lActual = str(lActual.decode('utf-8')).split('\n')
        self.assertEqual(lActual, lExpected)

As you can see the test function executes subprocess and checks that nothing is printed to stdout. I have run the some command in shell and it does print the output:

entity entity
entity entity1
entity is
end end
end entity
end entity1
port port
port (

Do you have any idea, what might be wrong?

[jeremiah-c-leary]

Instead of the multiple, let's focus on the single test case. I was wondering if it could have been an issue with the version of y'all, but since both the json and yaml tests fail it could not be the issue.

What do you get for output when you run the single test? Meanwhile I will look into the multiple test. It is interesting that the non reversed test passed...and the single test fails.

[jeremiah-c-leary]

So the tests that are failing use config3.json and config3.yaml. The multiple config tests that are passing uses config3 first and then config4. The reverse tests use config4 and then config3. The yaml and json files are the same so that further exonerates the version on yaml.

[jeremiah-c-leary]

Config3 disables port_007. Config4 enables port_007.

When passed in this order: config3 then config4, port_007 is enabled.

When passed in this order: config4 then config3, port_007 is disabled.

The only tests that fail are those that disable port_007.

[jeremiah-c-leary]

Looking at the output again, it is almost as if there are print statements outputting lines from the file.

Could you grep for print in your code base? You will get some. I could do the same and we could compare.

[m-kru]

vsg/__main__.py
38:        parser.print_help()
54:            print('Error in configuration file: ' + sFileName)
152:    version.print_version(commandLineArguments)

vsg/rule_list.py
28:        print('ERROR: specified local rules directory ' + sDirectoryName + ' could not be found.')
167:            print(sFileTitle)
168:            print('=' * len(sFileTitle))
173:                    print('Phase ' + str(phase) + '... Reporting')
180:                    print('Phase ' + str(phase) + '... Not executed')
183:            print('=' * len(sFileTitle))
184:            print('Total Rules Checked: ' + str(self.iNumberRulesRan))
185:            print('Total Violations:    ' + str(iFailures))

vsg/version.py
4:def print_version(oCommandLineArguments):
7:        print('VHDL Style Guide (VSG) version ' + str(version))

vsg/rule.py
60:                    print(sOutputString)

vsg/tests/utils.py
2:import pprint
9:        print('{0:5d} | {1:s}'.format(iLineNumber + iIndex, oFile.lines[iLineNumber + iIndex].line))
18:def print_attributes(oLine):
19:    pp = pprint.PrettyPrinter(indent = 4)
20:    pp.pprint(oLine.__dict__)

vsg/tests/rule_list/test_rule_list.py
17:            print(oRule.name + '_' + oRule.identifier)

vsg/tests/procedure/test_fix_rule_procedure.py
44:#        print(self.oFile.lines[97].__dict__)
45:#        print(self.oFile.lines[98].__dict__)

vsg/tests/vhdlFile/test_vhdlFile_instantiation_assignments.py
89:#        utils.print_attributes(oFileGeneric.lines[81])

[jeremiah-c-leary]

That is bizarre. None of those would produce your output.

Is that output when you ran the reverse_multiple or the single? If it was the reverse, could you paste what the single outputs?

[jeremiah-c-leary]

This is what I get when I grep for print:

./vsg/rule_list.py:28:        print('ERROR: specified local rules directory ' + sDirectoryName + ' could not be found.')
./vsg/rule_list.py:159:        Prints out violations to stdout.
./vsg/rule_list.py:167:            print(sFileTitle)
./vsg/rule_list.py:168:            print('=' * len(sFileTitle))
./vsg/rule_list.py:173:                    print('Phase ' + str(phase) + '... Reporting')
./vsg/rule_list.py:180:                    print('Phase ' + str(phase) + '... Not executed')
./vsg/rule_list.py:183:            print('=' * len(sFileTitle))
./vsg/rule_list.py:184:            print('Total Rules Checked: ' + str(self.iNumberRulesRan))
./vsg/rule_list.py:185:            print('Total Violations:    ' + str(iFailures))
./vsg/__main__.py:38:        parser.print_help()
./vsg/__main__.py:54:            print('Error in configuration file: ' + sFileName)
./vsg/__main__.py:152:    version.print_version(commandLineArguments)
./vsg/tests/procedure/test_fix_rule_procedure.py:44:#        print(self.oFile.lines[97].__dict__)
./vsg/tests/procedure/test_fix_rule_procedure.py:45:#        print(self.oFile.lines[98].__dict__)
./vsg/tests/rule_list/test_rule_list.py:17:            print(oRule.name + '_' + oRule.identifier)
./vsg/tests/utils.py:2:import pprint
./vsg/tests/utils.py:9:        print('{0:5d} | {1:s}'.format(iLineNumber + iIndex, oFile.lines[iLineNumber + iIndex].line))
./vsg/tests/utils.py:18:def print_attributes(oLine):
./vsg/tests/utils.py:19:    pp = pprint.PrettyPrinter(indent = 4)
./vsg/tests/utils.py:20:    pp.pprint(oLine.__dict__)
./vsg/tests/vhdlFile/test_vhdlFile_instantiation_assignments.py:89:#        utils.print_attributes(oFileGeneric.lines[81])
./vsg/tests/vsg/test_vsg.py:189:        print returnCode
./vsg/version.py:4:def print_version(oCommandLineArguments):
./vsg/version.py:7:        print('VHDL Style Guide (VSG) version ' + str(version))
./vsg/rule.py:60:                    print(sOutputString)

[m-kru]

I have reinstalled vsg and all tests pass. I have no idea what was wrong, but it is fixed :). I will work on case rule now.

[jeremiah-c-leary]

awesome. That was getting very weird.

[jeremiah-c-leary]

Hey m-kru,

Do you think we can close this issue?

[m-kru]

Oh, sure I though it was already closed.