Positional info [SL-2197] #106

P0lip · 2019-03-22T20:44:00Z

Please check if the PR fulfills these requirements

Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

What kind of change does this PR introduce?

fixes #92

Range is now attached to a single validation object, therefore it's possible to see where the issue/error actually occurs in a file (when using Spectral using API)
Moreover, CLI makes use of them to display line and column as shown in the screenshot below

What is the current behavior?

Range is not included (API usage) and positional info is not displayed (CLI usage).

If this is a feature change, what is the new behavior?

See the screenshot above.

Does this PR introduce a breaking change?

No changes required!

Other information

(e.g. detailed explanation, related issues, links for us to have context, eg. stackoverflow, issues outside of the repo, forum, etc.)

philsturgeon · 2019-03-25T11:30:56Z

RE the chat we had elsewhere about showing file path or JSON path: I think we should include both in JSON, and only show file path in the CLI. Most users will be really excited to be able to CTRL/CMD+Click the file path and open it right up. If folk need the path information it will be available in JSON and other formats as we grow them out, and if users request we find a way to include it in stylish we can do that later.

Does that sound good?

P0lip · 2019-03-25T11:31:35Z

@philsturgeon
Sounds good to me!
I'm adding filenames at the very moment :)

philsturgeon · 2019-03-25T11:32:43Z

Awesome! This means v2.0 is very close.

src/cli/commands/lint.ts

P0lip · 2019-03-25T14:45:15Z

src/spectral.ts

@@ -25,9 +26,9 @@ export class Spectral {
    this.resolver = opts && opts.resolver ? opts.resolver : new Resolver();
  }

-  public async run(target: object): Promise<IRuleResult[]> {
+  public async run(target: object, parserMeta?: IParserMeta): Promise<IRuleResult[]> {


parserMeta is needed to calculate lines and columns for given JSON Path.

Ideally, we could have a single argument accepting IParserResult, but that would be breaking and would force users to use parseWithPointers functions.

If parserMeta is provided, range property (lines and columns) is attached to a single result and hence can be displayed in the output (CLI) or consumed programmatically (API).

@philsturgeon @tbarn your thoughts on that?
We will need to document it for sure.

What is this thing for?

Also, IRange (range property) follows LSP standard and hence lines and characters (columns) are 0-based.

@philsturgeon It's needed by getJsonPathForPosition

I know the above looks slightly weird, but it preserves the old behavior.
It simply degrades gracefully - if no parserMeta is provided, all validation results will still have all the properties except range (and source - name of the file).

We could have a single argument, but we'd force user to use parseWithPointers function in such case, which is something we most likely want to avoid.
The other possible solution is to have another method, i.e. runParsed or similar, but IMHO this would introduce even bigger confusion to end-users.

@tbarn @philsturgeon @marbemac thoughts?
Note, The solution is also briefly described in the readme above.
Let me know what your thoughts are.

Edit:
Alternatively, we could also make run accept strings only (that'd be my favorite solution)

Yeah, sounds good to me.
What about S^2 (I hope ya know what I mean)? I'd like to avoid reparsing as much as possible in this particular case as this may degrade performance.

Apart from that, we'd still need to figure out some solution for CLI. CLI passes an already parsed content and I assume the result of safeStringify wouldn't equal the actual content of a file.

Or did you mean to have the following implementation?

run(target: IParserAstResult | object) { // if plain object, reparse }

What about something like this?

Not sure we even need to document the IParserAstResult right now - can use internally and from studio for now.

run(target: IParserAstResult | object | string) { let parsed = target; if (!isParserAstResult(target)) { parsed = parseWithPointers(safeStringify(target)); } // ...resolve parsed, etc }

The above looks good to me.

Yeah having it there but hidden from docs is fine with me.

tbarn · 2019-03-25T15:23:13Z

I'm getting an error when I do yarn build:

➜  spectral git:(feat/positional-info) yarn build
yarn run v1.13.0
$ sl-scripts build && oclif-dev manifest && node ./scripts/prepare-cli
building......
src/linter.ts:6:10 - error TS2305: Module '"../../../../../Users/taylorbarnett/Stoplight/spectral/node_modules/@stoplight/yaml"' has no exported member 'getLocationForJsonPath'.

6 import { getLocationForJsonPath } from '@stoplight/yaml';
           ~~~~~~~~~~~~~~~~~~~~~~


Found 1 error.
building...... done
error Command failed with exit code 2.

P0lip · 2019-03-25T15:23:41Z

@tbarn make sure to run yarn before (dependencies have been updated).

tbarn · 2019-03-25T15:25:03Z

@P0lip I did but I missed that it had an error too:

error sane@4.1.0: The engine "node" is incompatible with this module. Expected version "6.* || 8.* || >= 10.*". Got "9.11.2"

what version of node should I be using?

P0lip · 2019-03-25T15:29:47Z

@tbarn
Either 8 or 10 (recommended as it's the latest LTS).
(alternatively, you can do yarn --ignore-engines)

In any case, I don't see a reason not to support Node 9, therefore, the issue you expected should be fixed.
We might need to find out why sane doesn't accept Node 6 and node 9.

tbarn · 2019-03-25T15:43:34Z

@P0lip I probably set my nvm to v9 to test something else. I just want to make sure we have these kinds of things recorded in the contributing doc for others.

I got it running, tested out a few different spec documents, and it looks great to me.

P0lip · 2019-03-25T16:36:50Z

@tbarn
Did yarn --ignore-engines work for you?
If so, it's a good candidate to be listed in some troubleshooting section until we fix the issue.

tbarn · 2019-03-25T18:31:26Z

Shared elsewhere, but I wanted to share here too. The bump on the stoplight/yaml package fixed the problem on v9.

README.md

package.json

philsturgeon · 2019-03-26T12:53:21Z

src/spectral.ts

-  public async run(target: object): Promise<IRuleResult[]> {
-    const resolvedTarget = (await this.resolver.resolve(target)).result;
-    return runRules(target, this.rules, this.functions, { resolvedTarget });
+  public async run(target: IParserMeta | object | string): Promise<IRuleResult[]> {


Instead of messing with the target argument, could we possibly add a second optional argument for this stuff?

then we can throw warnings saying "We cannot give you any positional information because you didnt give us enough information" and the output can be ?? or something? Does that make any sense?

Yeah, we had that done this way initially but was changed as discussed here #106 (comment)
In general, I'm not opinionated on that at all and I'm happy to go with whatever is more intuitive for users.

Ah, my comment saying Yeah having it there but hidden from docs is fine with me. was written from mobile and I hadn't seen marcs suggestion.

Ok, so the name IParserMeta is whats throwing me. Marcs example makes it clear that it can either be a parsed AST object, or a generic object, and if its the generic object then its not gonna work with line numbers and stuff. That's all fine if we use a name a bit more like his.

Metadata is usually a different thing to data, so if it has meta in the name I'd expect it to be a different argument.

@philsturgeon
Yeah, naming is certainly not my strongest asset 😆

I can name it IParsedAst if it makes everything more clear.

Renamed to IParsedResult since it also has some different properties than just ast, so this could be confusing.
Let me know what you think @philsturgeon

and if its the generic object then its not gonna work with line numbers and stuff

It should still work if go with original suggestion. Basically, if it's a plain object we stringily it and parse it with our parser to get positional info:

run(target: IParserAstResult | object | string) { let parsed = target; if (!isParserAstResult(target)) { parsed = parseWithPointers(safeStringify(target)); } // ...resolve parsed, etc }

So no compromise for end user passing in plain object or stringified object.

Yeah, we reparse it therefore some ranges are attached. Initially, when we didn't reparse the object, ranges were missing.

philsturgeon · 2019-03-26T15:18:31Z

Perfect!

* feat: implement positional info * feat: show filepath * test: update snapshots * feat: use resolve instead of join * fix: show message if rule summary is missing * chore: document code, update readme and rename uri to source * refactor: exclude source if undefined * refactor: enhance run * feat: sort results by line * fix: summary is yellow in some cases * feat: rename IParsedResult -> IParsedResult

philsturgeon mentioned this pull request Mar 25, 2019

[SL-2196] CLI #105

Merged

2 tasks

P0lip force-pushed the feat/positional-info branch from c8d805e to 2b4576a Compare March 25, 2019 14:36

P0lip changed the base branch from master to release/2.0 March 25, 2019 14:36

philsturgeon reviewed Mar 25, 2019

View reviewed changes

src/cli/commands/lint.ts Outdated Show resolved Hide resolved

P0lip commented Mar 25, 2019

View reviewed changes

P0lip marked this pull request as ready for review March 25, 2019 14:46

P0lip requested a review from tbarn March 25, 2019 14:46

P0lip self-assigned this Mar 25, 2019

P0lip commented Mar 25, 2019

View reviewed changes

README.md Outdated Show resolved Hide resolved

philsturgeon reviewed Mar 26, 2019

View reviewed changes

package.json Outdated Show resolved Hide resolved

philsturgeon reviewed Mar 26, 2019

View reviewed changes

P0lip force-pushed the feat/positional-info branch from fddf53f to 4636c76 Compare March 26, 2019 14:22

P0lip added 8 commits March 26, 2019 15:23

feat: implement positional info

0af857e

feat: show filepath

5b4dcae

test: update snapshots

d1adcdc

feat: use resolve instead of join

53070e5

fix: show message if rule summary is missing

f572e29

chore: document code, update readme and rename uri to source

da68f96

refactor: exclude source if undefined

359f0ac

refactor: enhance run

f491231

P0lip added 2 commits March 26, 2019 15:23

feat: sort results by line

8b719b9

fix: summary is yellow in some cases

9d91ed4

P0lip force-pushed the feat/positional-info branch from 4636c76 to 9d91ed4 Compare March 26, 2019 14:23

P0lip requested a review from marbemac March 26, 2019 14:24

feat: rename IParsedResult -> IParsedResult

e31ef49

philsturgeon approved these changes Mar 26, 2019

View reviewed changes

P0lip merged commit 5d52318 into release/2.0 Mar 26, 2019

P0lip deleted the feat/positional-info branch March 26, 2019 16:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Positional info [SL-2197] #106

Positional info [SL-2197] #106

P0lip commented Mar 22, 2019 •

edited

philsturgeon commented Mar 25, 2019

P0lip commented Mar 25, 2019

philsturgeon commented Mar 25, 2019 via email

P0lip Mar 25, 2019

philsturgeon Mar 25, 2019

P0lip Mar 25, 2019

P0lip Mar 25, 2019

P0lip Mar 25, 2019 •

edited

P0lip Mar 25, 2019 •

edited

P0lip Mar 26, 2019 •

edited

marbemac Mar 26, 2019

P0lip Mar 26, 2019

philsturgeon Mar 26, 2019

tbarn commented Mar 25, 2019

P0lip commented Mar 25, 2019

tbarn commented Mar 25, 2019 •

edited

P0lip commented Mar 25, 2019 •

edited

tbarn commented Mar 25, 2019

P0lip commented Mar 25, 2019

tbarn commented Mar 25, 2019

philsturgeon Mar 26, 2019

P0lip Mar 26, 2019 •

edited

philsturgeon Mar 26, 2019

P0lip Mar 26, 2019

P0lip Mar 26, 2019

marbemac Mar 26, 2019 •

edited

P0lip Mar 26, 2019

philsturgeon commented Mar 26, 2019 via email

Positional info [SL-2197] #106

Positional info [SL-2197] #106

Conversation

P0lip commented Mar 22, 2019 • edited

Please check if the PR fulfills these requirements

What kind of change does this PR introduce?

What is the current behavior?

If this is a feature change, what is the new behavior?

Does this PR introduce a breaking change?

Other information

philsturgeon commented Mar 25, 2019

P0lip commented Mar 25, 2019

philsturgeon commented Mar 25, 2019 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

P0lip Mar 25, 2019 • edited

Choose a reason for hiding this comment

P0lip Mar 25, 2019 • edited

Choose a reason for hiding this comment

P0lip Mar 26, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbarn commented Mar 25, 2019

P0lip commented Mar 25, 2019

tbarn commented Mar 25, 2019 • edited

P0lip commented Mar 25, 2019 • edited

tbarn commented Mar 25, 2019

P0lip commented Mar 25, 2019

tbarn commented Mar 25, 2019

Choose a reason for hiding this comment

P0lip Mar 26, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marbemac Mar 26, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philsturgeon commented Mar 26, 2019 via email

P0lip commented Mar 22, 2019 •

edited

P0lip Mar 25, 2019 •

edited

P0lip Mar 25, 2019 •

edited

P0lip Mar 26, 2019 •

edited

tbarn commented Mar 25, 2019 •

edited

P0lip commented Mar 25, 2019 •

edited

P0lip Mar 26, 2019 •

edited

marbemac Mar 26, 2019 •

edited