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

MatchError's should include the filename #1806

Merged
merged 3 commits into from
Jan 12, 2022

Conversation

cognifloyd
Copy link
Contributor

@cognifloyd cognifloyd commented Jan 11, 2022

Whenever possible, we should include the filename and linenumber in the MatchError.

I also used the LINE_NUMBER_KEY const var instead of using "__line__".

This is minor and should not impact anyone now, but it makes fixing identified errors easier in #1805 (this was extracted from that PR).

@cognifloyd cognifloyd requested a review from a team as a code owner January 11, 2022 15:07
@cognifloyd cognifloyd requested review from relrod, cidrblock and priyamsahoo and removed request for a team January 11, 2022 15:07
@cognifloyd
Copy link
Contributor Author

These rules do not provide linenumber because identifying the line would be fiddly or misleading:

  • Meta rules can identify where galaxy_info is, but calculating line numbers for the contents of galaxy_info would be annoying and passing a linenumber for galaxy_info would be misleading.
    • MetaMainHasInfoRule
    • MetaTagValidRUle
    • MetaVideoLinksRule

@cognifloyd
Copy link
Contributor Author

I also used the LINE_NUMBER_KEY const var instead of using "__line__".

@cognifloyd
Copy link
Contributor Author

rebased on main

@ssbarnea
Copy link
Member

@cognifloyd Please search for __line__ and __file__ and replace the few remaining occurences with the newly added key so we have it in a single place, consistent.

@cognifloyd
Copy link
Contributor Author

Yup. I searched for __line__ and __file__. There is one other use of "__line__" but it is semantically different (an attribute instead of a dict key), and its use is isolated to one function in utils, so I didn't change that.

@cognifloyd
Copy link
Contributor Author

The other instances of __file__ are the magic python var, not string constants.

@cognifloyd
Copy link
Contributor Author

Here are the other uses of the string '__line__' (referring to an attribute instead of a key):
https://github.com/ansible-community/ansible-lint/blob/6df35484845627cb117320a221c7f3aac07f85b4/src/ansiblelint/utils.py#L715-L739

Copy link
Contributor

@tadeboro tadeboro left a comment

Choose a reason for hiding this comment

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

Thank you, @cognifloyd, for your contribution!

@ssbarnea ssbarnea merged commit 43a9243 into ansible:main Jan 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants