-
Notifications
You must be signed in to change notification settings - Fork 660
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
change: add file: Lintable argument to <rule_class>.match{,task} methods #1535
change: add file: Lintable argument to <rule_class>.match{,task} methods #1535
Conversation
@ssato I do agree that Lintable should be exposed, at least for the matchtask. For match(), it gets quite complicated and my plans were to deprecate and remove it. I personally see no good reason why a rule should look at a text line, especially as we are no longer doing YAML validations ourselves. IMHO, all the checks should be done using the already loaded data, not the source file. Example:
The construct above represents the same thing as The really tricky issue is that both of these changes will break all custom rules people may have, so we would need to do them in v6. How about a compromise for matchtask that should avoid breaking users: add the Lintable argument as optional (second arg). I can see how a change like that would work, as I would not have to worry about undesired side-effects. |
@ssbarnea Thanks a lot for your feedbacks!
Absolutely right. I removed that part modify .match() from the change to simplify it.
Also I fully agree with you. I updated to allow calling matchtask() w/o the file: Lintable argument by setting it to None by default. I pushed another commit brings these updates to make these changes easier to understand. |
9076ebf
to
960687f
Compare
src/ansiblelint/rules/__init__.py
Outdated
@@ -114,7 +114,7 @@ def matchtasks(self, file: Lintable) -> List[MatchError]: | |||
|
|||
if 'action' not in task: | |||
continue | |||
result = self.matchtask(task) | |||
result = self.matchtask(file, task) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clearly these calls use incorrect order or arguments. I hope they fail the linting as I do not want to see others doing the same mistake.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did spot one error you need to fix, but in general it looks ok.
Run the "tox -e lint,py" locally and once all CI jobs are fixed it should be ready to merge.
Add 'file: Lintable' argument to matchtask method of the rule classes which is None by default, because there are cases that it is necessary to do some analyses depends on the context, that is, file: Lintable, like the followings. - want to restrict tasks in 'main.yml' use include_tasks only - want to restrict the level of inclusions with include_tasks to 2 Signed-Off-By: Satoru SATOH <satoru.satoh@gmail.com>
960687f
to
98e8e06
Compare
Thanks for your quick feedback! I fixed all of the issues AFAIK, squashed commits and pushed with -f for merge. |
Add file: Lintable argument to match and matchtask methods of the rule
classes because there are cases that it is necessary to do some analyses
depends on the context, that is, file: Lintable, such like the followings.
Signed-Off-By: Satoru SATOH satoru.satoh@gmail.com