[build] rework tox, PEP-518, pre-commit and more

[alonbl]

Hello @jeremiah-c-leary ,

Thank you so much of maintaining this project with great response times and quality.

I would like to help a bit by adding some static code analysis to this project, after all this what you provide to others :)

This also upgrades project to recent python tooling.

This patch series is organized from the most trivial commit to the most intrusive to help you review. GitHub is doing very bad job in making code review possible as it is branch based and not patch based, you will probably review each commit offline and comment individually.

Building is simple just use tox, on ubuntu install using apt install tox on Windows using pip install tox, everything runs via tox, the style check, unittest, wheel, docs. Build outputs and reports are written into build.out, wheel is written into dist.

Notable changes:

• The version is modified to use setuptools-git-versioning no longer a proprietary process.
• The tests should not be part of the module as they are not distributed, they moved out to root.

Issues:

• docstrings: you have places in which docstrings contains markdown, I am unsure this is legit, pycoverage complains, I suggest removing these.
• tests: There are couple of tests that are not working with pytest, I think it is related to assumption that tests are serial and not parallel or the fact that the tests creates files within the source tree. These tests are disabled in tests: temporary disable tests that do not run with pytest until we sort out why.
• isort: There are couple of module that abuse the python import mechanism, I excluded them from isort, I think they should be removed in favor of explicit import (review pre-commit isort).
• json: there are some configuration files that have .json extension but are not json compliant, I am unsure how you parse config, I recommend to use standard notation (review pre-commit json).
• trailing-whitespace: I excluded some files, I am unsure all should be excluded for whitespace tests, please fix these that should not contain whitespaces (review pre-commit trialing-whitespace).
• end-of-file: inconsistent end-of-file of files, when I fix this for .vhd files many tests fail, I am unsure how to fix this (review pre-commit end-of-file), text files should embed end-of-line at their end when written by the tool.

These issues may be resolved after we confirm this set does not introduce regressions.

After resolving the above issues, you may consider to enable the following:

• lint - lots of issues, many of them will assist you to reduce code complexity.
• pep8 - after lint is applied you can enable this, it will detect some minor issues on top.

As these changes are difficult to rebase, if you consider merging this please review quickly so we resolve any issues and questions you may have.

Best Regards, Alon Bar-Lev

[jeremiah-c-leary]

Evening @alonbl ,

Thank you so much of maintaining this project with great response times and quality.

Thanks for the kind words.

I would like to help a bit by adding some static code analysis to this project, after all this what you provide to others :)

That would be awesome. Anything to help improve the code base would be appreciated. It will help in maintaining the project in the future.

This patch series is organized from the most trivial commit to the most intrusive to help you review. GitHub is doing very bad job in making code review possible as it is branch based and not patch based, you will probably review each commit offline and comment individually.

I appreciate the way you have made your commits. It is easy to walk through each commit because each one was very targeted in the issue it was resolving.

Building is simple just use tox, on ubuntu install using apt install tox on Windows using pip install tox, everything runs via tox, the style check, unittest, wheel, docs. Build outputs and reports are written into build.out, wheel is written into dist.

I think I need to get python 3.10 installed before I can run tox. I get the following error:

tox.tox_env.errors.Skip: could not find python interpreter with spec(s): py310

The version is modified to use setuptools-git-versioning no longer a proprietary process.

If that makes versioning easier, that would be fantastic. I really struggled to try to get that working.

The tests should not be part of the module as they are not distributed, they moved out to root.

100% agree. I have wanted to do this for the longest time, but never had the energy. Thank you.

Issues: ... These issues may be resolved after we confirm this set does not introduce regressions.

Agree. Let's ensure your current commits are good before tackling anything else.

As these changes are difficult to rebase, if you consider merging this please review quickly so we resolve any issues and questions you may have.

Would it be a good idea to merge this set of changes or wait until all the issues are resovled?

I will continue to review your commits and make any comments.

--Jeremy

[alonbl]

Hi,

Building is simple just use tox, on ubuntu install using apt install tox on Windows using pip install tox, everything runs via tox, the style check, unittest, wheel, docs. Build outputs and reports are written into build.out, wheel is written into dist.

I think I need to get python 3.10 installed before I can run tox. I get the following error:

Yes, I forgot this part, I pushed a support for 3.10, 3.11, 3.12. tox make it very easy to support multiple versions and also build this in parallel in CI.

Would it be a good idea to merge this set of changes or wait until all the issues are resovled?

Of course, this is why the series is built like this, you can merge each by order and I will rebase this request until we finish with all.

Regards,

[jeremiah-c-leary]

Morning @alonbl ,

I have python 3.10 installed now and can run tox. I have some updates for codespell, specifically the synopsis to synopsys change.

Is there any reason I should not commit those changes to the pull request?

--Jeremy

[alonbl]

Is there any reason I should not commit those changes to the pull request?

I like perfect commits before merging into master, this means that codespell changes should go into the codespell patch (rebase -i, modify) then pushed forced into the branch. I am not an expert in github... never forced pushed into pull request... I use gerrit which supports all these features.

maybe best is to create a branch in this repo for us to work on for a while.

but it should serve you, so do what you think is right, or just send me these modification and I will embed them into the chain.

[alonbl]

I have python 3.10 installed now and can run tox. I have some updates for codespell, specifically the synopsis to synopsys change.

what python version do you usually use? I can add this to tox, I updated this branch to support python-3.10,3.11,3.12

[jeremiah-c-leary]

Morning @alonbl ,

Are you keeping separate patch files for each commit?

I would normally just commit any changes to the pull request, but I could provide a patch of the changes I would like.

This would be an example patch for the codespell commit: codespell.patch

--Jeremy

[alonbl]

I would normally just commit any changes to the pull request, but I could provide a patch of the changes I would like.

I use git rebase -i master and edit the designated commit, preform the change and then git rebase --continue to rebase the chain. This is how the history is kept clean, each patch is doing exactly one task, no back and forth fixes. Just like, for example, how you submit patchset via email to linux or other projects, you reach perfect set before merge.

I am aware it is not trivial to work this way.... and to make it worse github thinks it uses cvs or svn and does not support the git flow...

I applied the codespell fixes into codespell and rebased the patchset on top of the new master, I see there were new commits which already created conflicts.

[jeremiah-c-leary]

I am aware it is not trivial to work this way.... and to make it worse github thinks it uses cvs or svn and does not support the git flow...

This is different than I normally work, but if you are good with me passing you patches then I am good. I just pulled down your latest commit and I see the latest codespell changes.

I will keep my patches in line with your commits. For example, I will not mix codespell updates with json updates. I will also identify which commit the patch applies to.

--Jeremy

[jeremiah-c-leary]

Morning @alonbl,

I believe this patch would work to exclude all vhdl file under the tests directory.:

style__remove_trailing_spaces.patch

Thanks,

--Jeremy

[alonbl]

Due to master and/or the codespell patch tests failed, I fixed it.

[alonbl]

Morning @alonbl,

I believe this patch would work to exclude all vhdl file under the tests directory.:

style__remove_trailing_spaces.patch

I would like to exclude only these tests that are designed to test vsg with trailing spaces, the rest should be fixed before commit by pre-commit hooks.

I believe the following tests are not intentionally with trailing spaces, if so I remove the trailing spaces and remove them from exclusion.

          tests/vhdlFile/full_type_declaration/classification_test_input\\.vhd|\
          tests/vhdlFile/simple_waveform_assignment/classification_test_input\\.vhd|\

[alonbl]

BTW: if a base commit is approved, please feel free to merge it into master, I will rebase this patchset on top.

[alonbl]

Morning @alonbl, I believe this patch would work to exclude all vhdl file under the tests directory.: style__remove_trailing_spaces.patch

I would like to exclude only these tests that are designed to test vsg with trailing spaces, the rest should be fixed before commit by pre-commit hooks.

I believe the following tests are not intentionally with trailing spaces, if so I remove the trailing spaces and remove them from exclusion.

          tests/vhdlFile/full_type_declaration/classification_test_input\\.vhd|\
          tests/vhdlFile/simple_waveform_assignment/classification_test_input\\.vhd|\

I succeeded in finding how to fix this and the test output, pushed a fix without them, trailing blanks handling is ok, exclusion reduced only to relevant tests.

[alonbl]

Remaining issues I do not know how to solve as I do not know the code:

• end-of-file-fixer - there should be no exceptions, please remove exceptions and try to resolve.
• tests: There are couple of tests that are not working with pytest, please enable and try to resolve.
• tabs: exclude only relevant vhd files in tests that test tab? fix the other to use spaces.

Remaining decisions:

• docstrings: you have places in which docstrings contains markdown, I am unsure this is legit, pycoverage complains, I suggest removing these.
• json: there are some configuration files that have .json extension but are not json compliant, I am unsure how you parse config, I recommend to use standard notation (review pre-commit json).
• isort: There are couple of module that abuse the python import mechanism, I excluded them from isort, I think they should be removed in favor of explicit import (review pre-commit isort).

[jeremiah-c-leary]

Morning @alonbl ,

I would like to exclude only these tests that are designed to test vsg with trailing spaces, the rest should be fixed before commit by pre-commit hooks.

I can not disagree with this position so the current exclusions are acceptable.

[jeremiah-c-leary]

Morning @alonbl ,

One fix for the end of file pre-commit:
end_of_file.patch

It turns out json.dumps does not add an end of line at the end of the json content.

I will start looking at the other exceptions.

--Jeremy

[jeremiah-c-leary]

@alonbl ,

Is there something I am missing when running pytest? I just issue pytest and I get the following errors:

_______________________________ ERROR collecting tests/vsg/test_vsg.py _______________________________
ImportError while importing test module '/home/jcleary/projects/vsg-master/tests/vsg/test_vsg.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
tests/vsg/test_vsg.py:11: in <module>
    from tests import utils
E   ImportError: cannot import name 'utils' from 'tests' (unknown location)

[jeremiah-c-leary]

Morning @alonbl ,

I can not seem to find the Sphinx Makefile in the docs directory. Wondering if you know why it is missing.

Thanks,

--Jeremy

[alonbl]

Hello @jeremiah-c-leary ,

Everything is migrated to tox :) You can see example of usage at CI[1].

I can not seem to find the Sphinx Makefile in the docs directory. Wondering if you know why it is missing.

In this case:

tox -e docs

Is there something I am missing when running pytest? I just issue pytest and I get the following errors:

You can see example of usage at CI[1], pytest is run automatically when you run tox you can run only tests and for specific python version using:

tox -e test-py311

[1] https://github.com/jeremiah-c-leary/vhdl-style-guide/actions/runs/8351168927

[alonbl]

It turns out json.dumps does not add an end of line at the end of the json content.

Thank you for finding this, applied.

[alonbl]

docstrings: you have places in which docstrings contains markdown, I am unsure this is legit, pycoverage complains, I suggest removing these.

Do you give me permissions to remove markdown from docstrings and resolve these warnings?

DeprecationWarning: invalid escape sequence XXXX

[jeremiah-c-leary]

Morning @alonbl ,

Here is a patch for the failing tests:

fixed_tests.patch

Although there is a missing __init__.py file in tests/attribute and tests/ieee directories. I did not see these added in the patch.

Do you give me permissions to remove markdown from docstrings and resolve these warnings?

I need to check how the docstrings get converted into restructured text first. I added the escape of the underscore for some reason.

--Jeremy

[jeremiah-c-leary]

Morning @alonbl ,

tabs: exclude only relevant vhd files in tests that test tab? fix the other to use spaces.

All the files in the following file should be excluded from checking for tabs:

allowed_tabs.txt

Every other file should have tabs removed.

--Jeremy

[jeremiah-c-leary]

tox -e docs

I must have something not setup correctly as I get the following error:

jcleary@DESKTOP-HV9NHA3:~/projects/vsg-master$ tox -e docs
ROOT: tox-gh-actions won't override envlist because tox is not running in GitHub Actions
.pkg: _optional_hooks> python /home/jcleary/projects/vsg-master/.tox/.tox/lib/python3.8/site-packages/pyproject_api/_backend.py True setuptools.build_meta
.pkg: get_requires_for_build_sdist> python /home/jcleary/projects/vsg-master/.tox/.tox/lib/python3.8/site-packages/pyproject_api/_backend.py True setuptools.build_meta
.pkg: build_sdist> python /home/jcleary/projects/vsg-master/.tox/.tox/lib/python3.8/site-packages/pyproject_api/_backend.py True setuptools.build_meta
docs: install_package> python -I -m pip install --force-reinstall --no-deps /home/jcleary/projects/vsg-master/.tox/.tmp/package/22/vsg-3.22.0.post25+git.0b2432e5.tar.gz
docs: commands[0]> sphinx-build -M html docs build.out/docs
Running Sphinx v7.1.2

Configuration error:
There is a programmable error in your configuration file:

Traceback (most recent call last):
  File "/home/jcleary/projects/vsg-master/.tox/docs/lib/python3.8/site-packages/sphinx/config.py", line 356, in eval_config_file
    exec(code, namespace)  # NoQA: S102
  File "/home/jcleary/projects/vsg-master/docs/conf.py", line 24, in <module>
    import vsg.version
  File "/home/jcleary/projects/vsg-master/vsg/version.py", line 7, in <module>
    @functools.cache
AttributeError: module 'functools' has no attribute 'cache'

docs: exit 2 (0.17 seconds) /home/jcleary/projects/vsg-master> sphinx-build -M html docs build.out/docs pid=6224
  docs: FAIL code 2 (8.84=setup[8.67]+cmd[0.17] seconds)
  evaluation failed :( (8.89 seconds)

which is odd because I can load functools by calling python directly:

jcleary@DESKTOP-HV9NHA3:~/projects/vsg-master/vsg$ python
Python 3.10.13 (main, Mar 17 2024, 08:38:35) [GCC 9.4.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import functools
>>> blah = functools.cache

Still looking into this.

--Jeremy

[alonbl]

Parallel tests are supported, see tests: create files in temporary location commit, it will save us many life hours waiting for build. I also found files that were probably added into git by mistake, see cleanup: remove unused files commit.

[alonbl]

Here is a patch for the failing tests:

Thanks! Applied.

Although there is a missing __init__.py file in tests/attribute and tests/ieee directories. I did not see these added in the patch.

I will add, this is the reason why you did not notice the issue.

Do you give me permissions to remove markdown from docstrings and resolve these warnings?

I need to check how the docstrings get converted into restructured text first. I added the escape of the underscore for some reason.

Please try to remember why, it is not standard, many tools in this new ecosystem complain about it.

[alonbl]

tabs: exclude only relevant vhd files in tests that test tab? fix the other to use spaces.

All the files in the following file should be excluded from checking for tabs:

This is too random, I would keep the *.vhd for now, please consider in future to move these tests to common package or have indicative name so that a generic exclusion filter may be applied.

[jeremiah-c-leary]

json: there are some configuration files that have .json extension but are not json compliant, I am unsure how you parse config, I recommend to use standard notation (review pre-commit json).

 40       - id: check-json
 41         # These are not json files, not sure what they are
 42         exclude: "^(\
 43           tests/vsg/config_error\\.json|\
 44           tests/vsg/read_configuration_files/config_w_file_list_globbing_w_individual_rule_config\\ 45           tests/vsg/read_configuration_files/config_w_file_list_w_individual_rule_config\\.json\
 46           )$"

The first JSON file is expected to fail the check as there is an intentional error in it to validate a test. The second and third files should pass.

This is the error reported:

tests/vsg/read_configuration_files/config_w_file_list_w_individual_rule_config.json: Failed to json decode (Expecting ',' delimiter: line 4 column 50 (char 121))
tests/vsg/read_configuration_files/config_w_file_list_globbing_w_individual_rule_config.json: Failed to json decode (Expecting ',' delimiter: line 4 column 51 (char 122))

This is the first file:

  1 {
  2   "file_list": [
  3     "tests/vsg/read_configuration_files/entity.vhd",
  4     "tests/vsg/read_configuration_files/arch.vhd": {
  5       "rule": {
  6         "rule_001": {
  7           "disable": true
  8         }
  9       }
 10     },
 11     "tests/vsg/read_configuration_files/package.vhd"
 12   ]
 13 }

and the comma is really on line 10.

So I believe we can keep the exclusions for the JSON files.

--Jeremy

[alonbl]

File "/home/jcleary/projects/vsg-master/.tox/docs/lib/python3.8/site-packages/sphinx/config.py", line 356, in eval_config_file

Do you want to support python3.8? I will work something out.

[jeremiah-c-leary]

Do you want to support python3.8? I will work something out.

I am not set on a particular version of python. I think the minimum requirement I have is 3.5, but at some point people would have to upgrade to a newer version.

So I am not apposed to switch to 3.10 or newer.

--Jeremy

[alonbl]

@jeremiah-c-leary: github does not support python < 3.7 can we lower the minimum from 3.5 to 3.7?

[jeremiah-c-leary]

@jeremiah-c-leary: github does not support python < 3.7 can we lower the minimum from 3.5 to 3.7?

I am good with that, but I imagine 3.7 support will be dropped at some point. Maybe we should just move to 3.8 or 3.10?

--Jeremy

[jeremiah-c-leary]

docstrings: you have places in which docstrings contains markdown, I am unsure this is legit, pycoverage complains, I suggest removing these.

I did a quick test and removed the escape character and it appears to render correctly. The doc strings need to match the corresponding restructured text files under docs. So I believe the doc strings can be updated.

--Jeremy

[alonbl]

@jeremiah-c-leary: github does not support python < 3.7 can we lower the minimum from 3.5 to 3.7?

I am good with that, but I imagine 3.7 support will be dropped at some point. Maybe we should just move to 3.8 or 3.10?

Done.

[alonbl]

docstrings: you have places in which docstrings contains markdown, I am unsure this is legit, pycoverage complains, I suggest removing these.

I did a quick test and removed the escape character and it appears to render correctly. The doc strings need to match the corresponding restructured text files under docs. So I believe the doc strings can be updated.

if you use the docstring to generate the markdown you should escape this during the processing, not in the docstring.

[jeremiah-c-leary]

if you use the docstring to generate the markdown you should escape this during the processing, not in the docstring.

I will work on fixing the docstring issue and ensuring the documentation is generated correctly.

--Jeremy

[jeremiah-c-leary]

Here is a patch for the docstring:

docstring.patch

There is still one issue I have to address.

--Jeremy

[alonbl]

Here is a patch for the docstring:

docstring.patch

There is still one issue I have to address.

Thanks, managed in docstring: remove unsupported escape sequences patch, please notice that these trivial changes should be on top of the set so you can marge them without risk, I rebase and modify the including what you send patches to keep series in logical order.

[alonbl]

I solved the end-of-file issues, all but the following:

• tests/context/test_main.py - seek TODO temporary workaround
• tests/severity/test_main.py - seek TODO temporary workaround

I guess you will be able to find root cause and remove the workaround.

[alonbl]

The json is invalid... it took me a while to see this as well:

  {
    "file_list": [
     "tests/vsg/read_configuration_files/entity.vhd",
      "tests/vsg/read_configuration_files/arch.vhd": {          <---- you cannot have dict key in a list
        "rule": {
          "rule_001": {
            "disable": true
          }
        }
      },
     "tests/vsg/read_configuration_files/package.vhd"
    ]
  }

The above is non standard the following is the same but standard:

   {
-    "file_list": [
+    "file_list": {
-      "tests/vsg/read_configuration_files/entity.vhd",
+      "tests/vsg/read_configuration_files/entity.vhd": null,
       "tests/vsg/read_configuration_files/arch.vhd": {
         "rule": {
           "rule_001": {
             "disable": true
           }
         }
       },
-      "tests/vsg/read_configuration_files/package.vhd"
+      "tests/vsg/read_configuration_files/package.vhd": null
-    ]
+    }
  }

[alonbl]

Status

Almost done :)

@jeremiah-c-leary work in progress:

• docstrings: you working on this

Decisions:

• json: there are some configuration files that have .json extension but are not json compliant, I am unsure how you parse config, I recommend to use standard notation (review pre-commit json).

Differ to after merge:

• isort: There are couple of module that abuse the python import mechanism, I excluded them from isort, I think they should be removed in favor of explicit import (review pre-commit isort).

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 94.01%. Comparing base (6694078) to head (181953b).

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #1141 +/- ## ========================================== - Coverage 96.49% 94.01% -2.48% ========================================== Files 1550 1557 +7 Lines 29321 29028 -293 Branches 0 3414 +3414 ========================================== - Hits 28292 27291 -1001 - Misses 1029 1303 +274 - Partials 0 434 +434 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[jeremiah-c-leary]

Evening @alonbl ,

json: there are some configuration files that have .json extension but are not json compliant, I am unsure how you parse config, I recommend to use standard notation (review pre-commit json).

That is bizarre. I use PyYAML to read in JSON files and before that I used the python json parser in the standard library. It baffles me that either would properly parse that syntax if it was wrong.

I put the JSON file in https://jsonlint.com/ and it failed.

It might be incorrect, but it does work.

Not sure what to do with this at the moment.

--Jeremy

[alonbl]

Hi @jeremiah-c-leary ,

I believe everything is ready for merge, it took more resources than I expected, I think the result will serve you well in future.

Please check everything on your side and tell me if you see any breakage.

I will open separate bugs for the remaining tasks.

Regards, Alon

[jeremiah-c-leary]

Please check everything on your side and tell me if you see any breakage.

Everything passes.

It looks like setup.py was removed, so how do I publish to pypi? I noticed a pyrproject.toml file.

--Jeremy

[alonbl]

It looks like setup.py was removed, so how do I publish to pypi? I noticed a pyrproject.toml file.

I never published anything at pypi, I believe twine is the tool[1][2].

The wheel is created at dist/ folder.

How have you published so far?

[1] https://realpython.com/pypi-publish-python-package/#upload-your-package [2] https://www.geeksforgeeks.org/how-to-publish-python-package-at-pypi-using-twine-module/

[jeremiah-c-leary]

This like was very helpful: https://packaging.python.org/en/latest/tutorials/packaging-projects/

With everything passing I can't see any reason not to merge this PR to master.

--Jeremy

[alonbl]

This like was very helpful: https://packaging.python.org/en/latest/tutorials/packaging-projects/

With everything passing I can't see any reason not to merge this PR to master.

Great :)

Have you checked docs and readthedocs? Is any previous functionality missing?

I can automate the twine also via tox if you send me what the exact command that is working for you.

[jeremiah-c-leary]

Have you checked docs and readthedocs? Is any previous functionality missing?

The only way I know if readthedocs is working is to commit to master and see if it breaks. Then attempt fixes on master until it works.

This merge failed:

I can automate the twine also via tox if you send me what the exact command that is working for you.

Can the release process be something the only runs when I want versus each time I run tox?

[alonbl]

Oh! Github squashed the changes into a single one(!) this is a bad behavior for this kind of changes, we cannot trace back issues. It should have created merge requests or leave them as-is.