The flag '--fix' in default pre-commit-hook entry

[larshb]

Environment

• Version: 3.21.0
• OS: Linux (irrelevant)

Describe the bug The pre-commit-hook run in CI lists files changed and successfull executions with --fix, but gives no information about which files actually changed (ie. 0 warnings, 0 errors). Running pre-commit locally also changes the files that need formatting instead of simply failing if any staged files needs formatting.

To Reproduce Steps to reproduce the behavior:

1. Install VSG into pre-commit
2. Git stage files with formatting not adhering to current rules, but possible to automatically fix
3. Git commit
4. Observe unstaged changes after running vsg --fix on all modified VHDL-files, but 0 warnings and 0 errors in log

Expected behavior For pre-commit (and especially in linting in CI) we expect to see which files are failing and why, not a list of all changed files with 0 warnings and 0 errors without any information on why the CI lint job fails.

Screenshots

Additional context --fix should be optionally added to pre-commit-config args:, not in the default configuration's entry:

[jeremiah-c-leary]

Evening @larshb ,

Thinking about this a little more, it would seem the --fix option should be optional and not included in the .pre-commit-hooks.yaml file.

I believe the following would work:

 - id: vsg
    name: fix VHDL style
    description: VHDL Style Guide
    entry: vsg --jobs=1
    language: python
    types:
      - file
    files: \.(vhd|vhdl)$

I would suggest using the -of summary option as that would give a more compact output.

I pushed an update to the issue-1139 branch. When you get a chance could you check it out and let me know if it is working for you.

Adding @alonbl who developed the pre-commit-hook for comments.

Regards,

--Jeremy

[alonbl]

Hello,

1. pre-commit style hook is expected to fix the file, just like black, isort, add-trailing-comma, etc... this is why --fix is required.

2. It is true that vsg does not record file that it modifies, however, pre-commit detects that files were changed during execution and abort with error. This behavior is similar to the behavior of other plugins such as black, isort, add-trailing-commas. a simple git status shows what has been modified.

This is the output one gets, it is self explanatory:

style run-test: commands[0] | pre-commit run --all-files
No-tabs checker..........................................................Passed
<snip>
trim trailing whitespace.................................................Passed
fix VHDL style...........................................................Failed
- hook id: vsg
- files were modified by this hook
ERROR: InvocationError for command .tox/style/bin/pre-commit run --all-files (exited with code 1)

If files were modified it only means developer pushed without checking, it does not important which as developer will run this on his workstation and fix everything before pushing the project (properly now) to origin.

3. --jobs=1 is provided as pre-commit is already iterating files and produce jobs, there is no reason to have jobs on top jobs.

4. I use --output_format=syntastic to provide a nicer summary.

Just for the record my .pre-commit-config.yaml looks like:

  - repo: https://github.com/jeremiah-c-leary/vhdl-style-guide
    rev: 3.22.0
    hooks:
      - id: vsg
        args:
          - --configuration=.vsg.yaml
          - --output_format=syntastic

I believe the pre-commit settings and vsg behavior is comply with the pre-commit best practices, there is no reason to modify anything. Having written that, you may consider adding a optional parameter such as --log-fixed-files or similar for this issue use case, people will be able to add this to args if they want to log modified file in case they do not use or do not know how to use git or would like to have non-standard plugin behavior.

Regards,

[jeremiah-c-leary]

Morning,

Having written that, you may consider adding a optional parameter such as --log-fixed-files or similar for this issue use case, people will be able to add this to args if they want to log modified file in case they do not use or do not know how to use git or would like to have non-standard plugin behavior.

Maybe add a pre-commit selection to the --output_format?

I believe the pre-commit settings and vsg behavior is comply with the pre-commit best practices, there is no reason to modify anything.

The black formatter does not have a report only option, unlike VSG. The intent was to use --fix before committing and then allow a CI tool to detect formatting issues on committed code. So I can see the appeal of wanting to remove the --fix option from the default behavior. In this way the user is required to run the --fix option manually. Although I can also see the appeal of including the --fix option because the code will automatically be updated.

It is true that vsg does not record file that it modifies, however, pre-commit detects that files were changed during execution and abort with error.

There are several possible outcomes when running with --fix ( the last three column indicate if something is reported based on the -of option):

File Violations	File Modified	Exit Code	Violations Reported	vsg	syntastic	summary
No violations	No	0	None	Yes	No	Yes
Only fixable violations	Yes	1	None	Yes	No	Yes
Only unfixable violations	No	1	Unfixable	Yes	Yes	Yes
Mix of fixable and unfixable violations	Yes	1	Unfixable	Yes	Yes	Yes

So there are two cases in which a file will not be modified and in one of the cases a violation was detected but the file was not modified. In the case where only unfixable violations exist, the exit code would be 1 and the violations would be reported. Depending on the output format chosen you might have to wade through a bunch of output to find the violations since git would not detect files where were not modified.

For pre-commit (and especially in linting in CI) we expect to see which files are failing and why, not a list of all changed files with 0 warnings and 0 errors without any information on why the CI lint job fails.

For CI, there is the option of using --junit which will create a JUnit XML file the CI tool can consume. It reports only files with violations and lists each violation.

I believe @larshb is requesting --fix to be moved to args: so it can be overridden:

 - id: vsg
    name: fix VHDL style
    description: VHDL Style Guide
    entry: vsg --jobs=1
    args:
       - --fix
    language: python
    types:
      - file
    files: \.(vhd|vhdl)$

Regards,

--Jeremy

[alonbl]

Morning,

Hi,

Maybe add a pre-commit selection to the --output_format?

I do not think it is necessary, nor it is pre-commit requirements, as all the other pre-commit hooks do not provide this.

I believe the pre-commit settings and vsg behavior is comply with the pre-commit best practices, there is no reason to modify anything.

The black formatter does not have a report only option, unlike VSG. The intent was to use --fix before committing and then allow a CI tool to detect formatting issues on committed code. So I can see the appeal of wanting to remove the --fix option from the default behavior. In this way the user is required to run the --fix option manually. Although I can also see the appeal of including the --fix option because the code will automatically be updated.

As you can see the default of black is fix, I would like the default of vsg to be the same, more arguments can be added, the default of all the other plugins are fix as well, this is the expected behavior. We can add --fix=False or any other notation to override the default behavior as python does support --prm=a --parm=b last wins.

So there are two cases in which a file will not be modified and in one of the cases a violation was detected but the file was not modified. In the case where only unfixable violations exist, the exit code would be 1 and the violations would be reported. Depending on the output format chosen you might have to wade through a bunch of output to find the violations since git would not detect files where were not modified.

Correct.

For pre-commit (and especially in linting in CI) we expect to see which files are failing and why, not a list of all changed files with 0 warnings and 0 errors without any information on why the CI lint job fails.

For CI, there is the option of using --junit which will create a JUnit XML file the CI tool can consume. It reports only files with violations and lists each violation.

I believe @larshb is requesting --fix to be moved to args: so it can be overridden:

This will result in adding --fix to every arg modification by users, having them surprised that --fix was removed, the default of all other components is to perform fix... I propose again to have --fix=False or --fix=0 or --no-fix or anything similar as an argument to override previously applied --fix.

[larshb]

First of all, thank you for the suggestions on output_format.

I did not realize in-place formatting is default and expected behavior for pre-commit-hooks.

We have other hooks that require an argument for this, such as

- repo: https://github.com/pocc/pre-commit-hooks
  rev: v1.3.5
  hooks:
  - id: clang-format
    stages: [pre-commit]
    args:
    - -i

, where -i is added to run in-place editing.

We'll have to reconsider our .pre-commit-hooks.yaml and possibly differentiate for local execution and CI-execution. CI may still fail due e.g. a bad merge conflict resolution or rebasing.

You may close this issue, if you deem it not to be a bug.

[alonbl]

@larshb I suggest to use tox to invoke the hook, the following is a simple tox.ini for the task:

[tox]
skipsdist = True
envlist = style

[testenv:style]
deps =
    pre-commit
commands =
    pre-commit run --all-files {posargs}

people run tox locally to perform the tests and CI runs the same, tox is available as system package in most distributions.

[jeremiah-c-leary]

Morning @larshb ,

You may close this issue, if you deem it not to be a bug.

Given the discussion thread I will close this issue.

Regards,

--Jeremy