Added the py, pydsl, and pyobjects interpreters for salt #60
Conversation
Disabled the executable checks on files since not all python is required to be executable
Hopefully for better identification :)
There was a problem hiding this comment.
sorry this is so delayed of a review, this change as-is isn't appropriate as it breaks some core functionality and introduces a major performance regression -- files should not be read unless checking for shebangs.
In this case, you don't actually have a shebang (which are only relevant for executable files), you have more of a "comment directive" which identify doesn't really have a concept of currently.
It might make sense to introduce a new approach to identifying things which can identify these "comment directive" type files. In this scenario, we'd only check for these directives for specific extensions (such as .sls) and not for every file.
| @@ -184,7 +219,7 @@ def test_parse_shebang_from_file_does_not_exist(): | |||
| def test_parse_shebang_from_file_nonexecutable(tmpdir): | |||
| x = tmpdir.join('f') | |||
| x.write_text('#!/usr/bin/env python', encoding='UTF-8') | |||
| assert identify.parse_shebang_from_file(x.strpath) == () | |||
| assert identify.parse_shebang_from_file(x.strpath) == ('python',) | |||
There was a problem hiding this comment.
this change is inappropriate -- this test is intentionally demonstrating that files that are non-executable are never checked for shebangs
There was a problem hiding this comment.
I have removed that functionality in this PR as well since it is absolutely not required for a python file to be executable. This logic makes identify flag python files that are not executable as not python
There was a problem hiding this comment.
So to respond to your comment since I removed that logic I have also removed the test for it
There was a problem hiding this comment.
yes, and I'm telling you that's unacceptable
There was a problem hiding this comment.
Ah I'm sorry I only saw the inline comment and not the comment to the PR itself :) my fault
OK, why do you say this is unacceptable?
I thought I had left a comment about possibly adding some logic for only looking at the content for specific file types but I don't actually see that comment in here so my apologies, but yes for performances sake I would go that way myself.
There was a problem hiding this comment.
ah, I had left the comment in #61
There was a problem hiding this comment.
Is there a specific way you want to continue with this? Conditional inspection would also be nice for .mk files since those are not only used for Makefiles but python config files for Check-MK's version of nagios.
There was a problem hiding this comment.
Maybe something similar to #63?
There was a problem hiding this comment.
Ah, nice! Ok, I'll try that out for these and update asap. Thanks
Disabled the executable checks on files since not all python is required
to be executable