pocc / pre-commit-hooks

C/C++ hooks to integrate with pre-commit
Apache License 2.0
323 stars 67 forks source link

file regex not read from the configuration file #43

Closed qq7te closed 2 years ago

qq7te commented 2 years ago

At line 28 of hooks/utils.py , the comment says that this would capture the target files from the configuration file, but it seems to me that in reality the only file regex that it uses are the ones that are hardcoded, right after those lines.

Is that an actual error and/or are there plans to fix that?

def set_file_regex(self):
        """Get the file regex for a command's target files from the .pre-commit-hooks.yaml."""
pocc commented 2 years ago

Good eye. You have a point that there shouldn't be unnecessary duplication.

Did you find a regex that didn't cover one of your files, or were you just reading the code?

qq7te commented 2 years ago

we have some ipp and tpp files which are part of our code (used for template function definitions instead of putting them inline in the hpp).
So , I needed to add those to the ones being covered, but it seems that the config file is ignored, and the only thing that matters is that code in utils.py .

I think that more than being "duplication" , the config is ignored, and the comments should admit it :slightly_smiling_face:

pocc commented 2 years ago

Thanks for bringing this to my attention. This should be fixed with tag v1.3.5. I've tested with both types: and files:, and local config overrides hook config. This commit contains:

I've added a recommendation to add .ipp and .tpp files as part of the pre-commit c++ type here: https://github.com/pre-commit/identify/issues/259. If there are other c-like languages that also use this filetype that you're aware of, please comment on that issue.

Please let me know if this fix works for your needs.

qq7te commented 2 years ago

The new changes look good!
So far, My only problem is that my system doesn't recognize those type tags.

I've upgraded pre-commit to version 2.16.0 (latest) and identify to version 2.4.0 (latest) The strange thing is that I keep getting this error even though everything is updated. Do I need to configure something? I can remove the types_or: filter and everything will be fine, so I do have a workaround for myself. But if this is a problem that others will encounter, it's probably best to sort it out now.

=====> Type tag 'cc' is not recognized.  Try upgrading identify and pre-commit?

the type tag 'c' works fine, that's the first in the list. All others seem to give problems. I've removed cc to find out that cpp was a problem... and so on.

pocc commented 2 years ago

Apologies - the tag v1.3.5 was pointed to the wrong commit. It should be fixed now. Correct commit is 9a9bbc0.

qq7te commented 2 years ago

Thank you, no more errors now. :slightly_smiling_face:

Not only that, but I'm able to use your v1.3.5 and override locally.

        types_or: [ ]
        files: \.(c|cc|cxx|cpp|h|hpp|tpp|ipp|hxx|m)$

That lets me add tpp and ipp while I wait for identify to accept our suggestion! I don't know if it's going to be obvious to other users that in order to use the local files: , you need to null out the types_or:

Thank you again, really impressed with the quick turnaround !

pocc commented 2 years ago

Merry Christmas to you too! If you find it useful, give it a :star: or recommend it.

I do not see these issues you are talking about.

With the test files in /tests/test_repo moved to /tmp/test_repo, and with this .pre-commit-config.yaml:

 fail_fast: false
 repos:
   - repo: /home/rj/code/pre-commit-hooks
     rev: v1.3.5
     hooks:
       - id: clang-format
         args: [--style=Google]
         types: [c++]

I see this behavior:

$ git add .
$ git commit
[INFO] Initializing environment for /home/rj/code/pre-commit-hooks.
[INFO] Installing environment for /home/rj/code/pre-commit-hooks.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
clang-format.............................................................Failed
- hook id: clang-format
- exit code: 1
err.cpp
====================
--- original
+++ formatted
@@ -1,2 +1,5 @@
#include <string>
-int main(){int i;return;}
+int main() {
+  int i;
+  return;
+}

Only the cpp file is formatted.

If we remove types: [c++] and add files:.c$ORtypes_or: [c]`, we instead see output for the c file:

$ git add .
$ git commit
clang-format.............................................................Failed
- hook id: clang-format
- exit code: 1
err.c
====================
--- original
+++ formatted
@@ -1,2 +1,5 @@
#include <stdio.h>
-int main(){int i;return;}
+int main() {
+  int i;
+  return;
+}

If you can reproduce this errant behavior, please provide version information for all important libraries (pre-commit, identify, OS, clang/llvm, etc).

qq7te commented 2 years ago

Yes, it's a strange case. I will put together a cleaner example. Right now I'm shutting down work for the holidays :) Will update in January. Thanks for the good work and the excellent support ! Hopefully I'll be able to have my entire company adopt these hooks.

pocc commented 2 years ago

Closing as this issue as it looks resolved and has been dormant for a couple of weeks.

qq7te commented 2 years ago

yes, sorry for dropping the ball on this one . I hope to still provide some more details on this in the future