-
Notifications
You must be signed in to change notification settings - Fork 30
DOC-13478 vale review capella api #43
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,7 +26,11 @@ experimental = YES | |
|
|
||
| attribute-missing = drop | ||
|
|
||
| Vale.Spelling = NO | ||
|
|
||
| [*.{adoc,md,txt}] | ||
| BasedOnStyles = Vale, write-good, proselint, Couchbase, Google | ||
|
|
||
| Vale.Spelling = NO | ||
| [test/fixtures/test.yml] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is only going to limit to files called test.yml. Is that going to work for where you want to deploy this? Shouldn't we just specify the yml file extension generally, instead?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, this is just a sample test.yml file, for the purpose of testing the infrastructure. The customized .vale.ini then lives in the content repo, currently a POC one https://github.com/couchbaselabs/docs-vale-openapi-test-poc/blob/master/cmd/cp-open-api/specs/.vale.ini That's because there's some mangling for specific files needing a specific view (again, to work around a Vale+Views+dasel bug/misfeature) which I don't think needs to go in the central .vale.ini.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh my, that .ini seems a little fragile - I hope that bug fix makes that easier to manage going forward. |
||
| BasedOnStyles = Vale, write-good, proselint, Couchbase, Google | ||
| View = OpenAPIInclude | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reading the Vale docs - you've only included one of the .yml files you added for defining a view? Does the other one not work?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's the only view that's relevant for this file. See the POC .vale.ini linked above for more examples |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| engine: dasel | ||
| scopes: | ||
| - expr: properties.all().property(description?) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this proper Dasel syntax? What are you trying to extract with this?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yup. It's essentially |
||
| type: md | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does md just make linting this easier? Just curious.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OpenAPI descriptions are specified as Markdown I think? |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| engine: dasel | ||
| scopes: | ||
| - expr: description | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So this might work for the very simple yml - do we want to make it more complex for the yml we're actually going to be linting? 🤔
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is an actual usecase! https://github.com/couchbasecloud/couchbase-cloud/blob/main/cmd/cp-open-api/specs/schemas/backups/GetBackupByIDResponse.yaml is an example of OpenAPI.yml (.properties.*.description) https://github.com/couchbasecloud/couchbase-cloud/blob/main/cmd/cp-open-api/specs/schemas/backups/Method.yaml is an example of this OpenAPIInclude.yml (.description) The ideal would be
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, didn't realize that Vale hadn't quite caught up to the latest Dasel. Makes sense then. |
||
| type: md | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| type: test | ||
| content: | ||
| foo: 1 | ||
| bar: 2 | ||
| description: | | ||
| First sentence. Second sentence. | ||
| Test couchbase. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,51 @@ | ||
| const {range, zip, escape} = require('lodash'); | ||
|
|
||
| // this test file is designed to be run with Mocha | ||
| const assert = require('assert') | ||
| const fs = require('fs') | ||
| const ok = specify | ||
| const { spawnSync } = require('node:child_process') | ||
|
|
||
| describe('check spans for Yaml', function () { | ||
| testSpans('test/fixtures/test.yml') | ||
| }) | ||
|
|
||
| describe('check spans for Asciidoc', function () { | ||
| testSpans('test/fixtures/basic.adoc') | ||
| }) | ||
|
Comment on lines
+13
to
+15
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this just for testing? Just curious why we have a test for checking for AsciiDoc.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, just:
|
||
|
|
||
| function testSpans(file) { | ||
| const vale = runVale(file) | ||
|
|
||
| const text = fs.readFileSync(file).toString() | ||
| const lines = text.match(/^.*?(\n|$)/gm) | ||
|
|
||
| for (const item of vale[file]) { | ||
| const line = lines[item.Line - 1] | ||
| const [from,to] = item.Span | ||
| const span = line.substr(from - 1, to-from+1) | ||
|
|
||
| ok(`Match ${item.Match}`, function () { | ||
| assert.equal(item.Match, span) | ||
| console.log({line, span, match: item.Match}) | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| function runVale(file) { | ||
| let vale | ||
| try { | ||
| vale = spawnSync( | ||
| 'vale', | ||
| [ | ||
| '--output', 'JSON', | ||
| file | ||
| ] | ||
| ) | ||
| } | ||
| catch (err) { | ||
| console.log("Failed to run vale", err) | ||
| } | ||
|
Comment on lines
+46
to
+48
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Damn, does Vale not give more specific errors to try and map onto, or is this just for a quick and dirty approach?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not expecting the vale invocation to fail, so this just passes the error back to us so we can understand what happened (usually it'll be because I got the command parameters wrong). |
||
|
|
||
| return JSON.parse(vale.stdout) | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious why we don't want to run spellcheck?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question!
There were a lot of false positives, and it made testing very noisy.
I think the correct thing would be to add a
vocabularies/CapellaOpenAPI.txtand update those such that there's a canonical list of words used. (Some of these might also go into our Couchbase.txt list, but I think it's worth keeping some hygiene between them.)I'd like to do this as a subsequent step, rather than making it a blocker though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to me. Our current vocabularies aren't very fleshed out, so might be worth investing some time into those in general.