Skip to content
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

Reimplement unamed-task rule as name[missing] #2263

Merged
merged 1 commit into from Aug 9, 2022

Conversation

ssbarnea
Copy link
Member

@ssbarnea ssbarnea commented Aug 7, 2022

This reimplementation will allow us to implement closely related extra checks such name[casing], name[imperative], name[unique], name[suffix].

Related: #2171 #2170 #2169

@github-actions github-actions bot added the bug label Aug 7, 2022
@ssbarnea ssbarnea requested a review from cidrblock August 7, 2022 19:37
@ssbarnea ssbarnea force-pushed the fix/name branch 2 times, most recently from bb4ef5e to ee1a8eb Compare August 9, 2022 09:54
@ssbarnea ssbarnea marked this pull request as ready for review August 9, 2022 09:59
@ssbarnea ssbarnea requested review from a team as code owners August 9, 2022 09:59
This reimplementation will allow us to implement closely related
extra checks.

Related: ansible#2171 ansible#2170 ansible#2169 ansible#2036
Copy link
Member

@cognifloyd cognifloyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dang. Sorry I didn't review before this was merged. I have a few questions to address in a follow-up.

src/ansiblelint/rules/unnamed_task.py Show resolved Hide resolved
) -> Union[bool, str, MatchError]:
if not task.get("name"):
return self.create_matcherror(
linenumber=task["__line__"], tag="name[missing]", filename=file
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use the LINE_NUMBER_KEY constant in ansiblelint.utils.LINE_NUMBER_KEY

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, this bypasses some of the enrichment that happens in matchtasks() like details and match.task.

if isinstance(result, MatchError):
if result.tag in skipped_tags:
continue
match = result
else:
message = None
if isinstance(result, str):
message = result
task_msg = "Task/Handler: " + ansiblelint.utils.task_to_str(task)
match = self.create_matcherror(
message=message,
linenumber=task[ansiblelint.utils.LINE_NUMBER_KEY],
details=task_msg,
filename=file,
)
match.task = task

I'm not sure where we should add that. Maybe matchtasks() should be refactored to update details if it is not set, and add match.task.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was aware about missing details but I was not sure what to include in them. The reality is that the generic "name" description is no longer relevant for various violations.

Probably we need to add some longer descriptions but we need to write the text for them.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#2277 should address my feedback here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants