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

Fix 'location' attribute for CodeclimateJSONFormatter #2897

Merged
merged 11 commits into from
Jan 16, 2023
Merged

Fix 'location' attribute for CodeclimateJSONFormatter #2897

merged 11 commits into from
Jan 16, 2023

Conversation

4ch1m
Copy link
Contributor

@4ch1m 4ch1m commented Jan 16, 2023

Hi there,

I believe the current implementation of the CodeclimateJSONFormatter is not quite correct.

Please have a look at this block ...

issue["location"] = {}
issue["location"]["path"] = self._format_path(match.filename or "")
issue["location"]["lines"] = {}
if match.column:
issue["location"]["lines"]["begin"] = {}
issue["location"]["lines"]["begin"]["line"] = match.linenumber
issue["location"]["lines"]["begin"]["column"] = match.column
else:
issue["location"]["lines"]["begin"] = match.linenumber

... which always generates a lines-object.

Furthermore the begin-attribute changes type (to either int or object) depending of the presence of a column-value. (Which makes automatic object-binding extremely complicated if not impossible when parsing the JSON-string with other languages/frameworks.)

Aside from that, the codeclimate docs state:

Locations refer to ranges of a source code file. A Location contains a path and a source range (expressed as lines or positions).

So it should either be a lines-object ...

{
  "path": "path/to/file.css",
  "lines": {
    "begin": 13,
    "end": 14
  }
}

... with a begin/end range, or a positions-object ...

{
  "path": "path/to/file.css",
  "positions": {
    "begin": {
      "line": 3,
      "column": 10
    },
    "end": {
      "line": 4,
      "column": 12
    }
  }
}

... with line/column values.

This PR should fix that.

Thanks for reading. 😄

@4ch1m 4ch1m requested a review from a team as a code owner January 16, 2023 16:10
@github-actions github-actions bot added the bug label Jan 16, 2023
@ssbarnea
Copy link
Member

@4ch1m Update PYTEST_REQPASS to include the test you added. Other than this it looks good to me. Thanks!

@4ch1m
Copy link
Contributor Author

4ch1m commented Jan 16, 2023

@4ch1m Update PYTEST_REQPASS to include the test you added. Other than this it looks good to me. Thanks!

Done. Thank you. 👍

@ssbarnea ssbarnea changed the title fix 'location' attribute for CodeclimateJSONFormatter Fix 'location' attribute for CodeclimateJSONFormatter Jan 16, 2023
@ssbarnea ssbarnea merged commit fd71b3f into ansible:main Jan 16, 2023
SkrrtBacharach added a commit to SkrrtBacharach/ale that referenced this pull request Apr 3, 2023
The JSON output format of ansible-lint has changed since
6.11.0. Issue locations can have either a 'positions' or
a 'lines' member, rather than just a 'lines' member as it
was before. This fix checks which member is present, and
passes that member name to subsequent dictionary lookups.

The error was caused by the following change:
ansible/ansible-lint#2897
SkrrtBacharach added a commit to SkrrtBacharach/ale that referenced this pull request Apr 6, 2023
The JSON output format of ansible-lint has changed since
6.11.0. Issue locations can have either a 'positions' or
a 'lines' member, rather than just a 'lines' member as it
was before. This fix checks which member is present, and
passes that member name to subsequent dictionary lookups.

The error was caused by the following change:
ansible/ansible-lint#2897
hsanson pushed a commit to dense-analysis/ale that referenced this pull request Apr 7, 2023
* Fix error from ansible-lint versions >=6.11.0.

The JSON output format of ansible-lint has changed since
6.11.0. Issue locations can have either a 'positions' or
a 'lines' member, rather than just a 'lines' member as it
was before. This fix checks which member is present, and
passes that member name to subsequent dictionary lookups.

The error was caused by the following change:
ansible/ansible-lint#2897

* Add ansible-lint test to check each type of ansible-lint issue json.

* Change long single-line JSON in ansible test into multiline JSON.

* Fix linting errors in ansible_lint.vim.
mnikulin pushed a commit to mnikulin/ale that referenced this pull request Nov 12, 2023
* Fix error from ansible-lint versions >=6.11.0.

The JSON output format of ansible-lint has changed since
6.11.0. Issue locations can have either a 'positions' or
a 'lines' member, rather than just a 'lines' member as it
was before. This fix checks which member is present, and
passes that member name to subsequent dictionary lookups.

The error was caused by the following change:
ansible/ansible-lint#2897

* Add ansible-lint test to check each type of ansible-lint issue json.

* Change long single-line JSON in ansible test into multiline JSON.

* Fix linting errors in ansible_lint.vim.
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