Clarify usage of other_locations key#57
Conversation
SPEC.md
Outdated
|
|
||
| Offsets, however are 0-based. A Position of `{ "offset": 4 }` represents the _fifth_ character in the file. Importantly, the `offset` is from the beginning of the file, not the beginning of a line. Newline characters (and all characters) count when computing an offset. | ||
|
|
||
| ### Other locations |
There was a problem hiding this comment.
This should be referenced & linked in the TOC, I think.
SPEC.md
Outdated
|
|
||
| ### Other locations | ||
|
|
||
| Other locations is an optional array of location objects |
There was a problem hiding this comment.
- Missing a period here.
locationshould be capitalized & linked to theLocationssection that describes the Location schema.
|
|
||
| Other locations is an optional array of location objects | ||
|
|
||
| ```json |
There was a problem hiding this comment.
Personally I don't think a full example here is informative or necessary. It's just an array of an object type described elsewhere, and the full Issue schema makes that clear.
If you think it's important to keep, though, please make the formatting consistent with other cases that describe a top-level key, e.g. the example in the Trace section. (Specifically, I'm referring to not having the wrapping {} around the whole thing, which make it seem like this whole snippet is a valid object, which it isn't in our schema.)
SPEC.md
Outdated
| } | ||
| ] | ||
| } | ||
|
|
SPEC.md
Outdated
| ### Locations | ||
|
|
||
| Locations refer to ranges of a source code file. A Location contains a `path`, a source range, (expressed as `lines` or `positions`), and an optional array of `other_locations`. Here's an example location: | ||
| Locations refer to ranges of a source code file. A Location contains a `path`, a source range, (expressed as `lines` or `positions`). Here's an example location: |
There was a problem hiding this comment.
Since we've fixed this by dropping the reference to other_location, we now have some grammar to clean up here. I think it should be "contains a path and a source range (", not "contains a path, a source range, ("
ABaldwinHunter
force-pushed
the
ashley/clarify-other-locations
branch
from
d73d9ab to
b4a2591
Compare
|
@wfleming updated! ready for another look. I took suggestions. Left the code snippet because I have a bias to prefer more documentation |
|
My .02 is that maybe we should remove |
SPEC.md
Outdated
| ### Locations | ||
|
|
||
| Locations refer to ranges of a source code file. A Location contains a `path`, a source range, (expressed as `lines` or `positions`), and an optional array of `other_locations`. Here's an example location: | ||
| Locations refer to ranges of a source code file. A Location contains a `path` and a source range, (expressed as `lines` or `positions`). Here's an example location: |
There was a problem hiding this comment.
I still don't think the comma after "range" here is grammatical: it makes it read as though the parenthetical applies to "a path and a source range", but in fact it's only referring to "a source range".
There was a problem hiding this comment.
whoops left that by accident. thanks
I don't think it's an undocumented internal extension: it's been part of the spec forever and is documented. I think the incorrect sentence under |
|
I think those are both fair points. I think for now clarifying the documentation we have seems like the most straightforward path, and considering removal could happen as well separately - since it's a more product-y question? |
It appears that codeclimate#20 intended to remove `other_locations` as superseded by `trace`, but it left the old entry (only) in the example JSON object. An expanded section describing `other_locations` was put back in codeclimate#57, that issue thread asserts that it is still valid and in use by Code Climate's duplication engine. These two fields certainly seem functionally redundant, hence codeclimate#56. Under assumption that both are used/accepted in implementations of the current version of the spec, this change restores presence of both in the reference but proposes deprecating `other_locations` and hence de-emphasizes its documentation.
4 participants
Addresses #56