-
Notifications
You must be signed in to change notification settings - Fork 26
style(spec): add out-of-line-enum rule APIC-416 #356
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
2d36490
9af3d59
67d8f9d
4b149ba
5e8757f
05a1a0e
b2a79a7
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 |
---|---|---|
@@ -1 +1 @@ | ||
7.1.0 | ||
7.2.0 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,11 @@ | ||
import { descriptionDot } from './rules/descriptionDot'; | ||
import { outOfLineEnum } from './rules/outOfLineEnum'; | ||
import { singleQuoteRef } from './rules/singleQuoteRef'; | ||
|
||
const rules = { | ||
'description-dot': descriptionDot, | ||
'out-of-line-enum': outOfLineEnum, | ||
'single-quote-ref': singleQuoteRef, | ||
}; | ||
|
||
export { rules }; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,44 @@ | ||
import type { Rule } from 'eslint'; | ||
|
||
import { isPairWithKey } from '../utils'; | ||
|
||
export const outOfLineEnum: Rule.RuleModule = { | ||
meta: { | ||
docs: { | ||
description: 'enum must be out of line', | ||
}, | ||
messages: { | ||
enumNotOutOfLine: 'enum is not out of line', | ||
}, | ||
}, | ||
create(context) { | ||
if (!context.parserServices.isYAML) { | ||
return {}; | ||
} | ||
|
||
return { | ||
YAMLPair(node): void { | ||
if (!isPairWithKey(node, 'enum')) { | ||
return; | ||
} | ||
// parent is mapping, and parent is real parent that must be to the far left | ||
if (node.parent.parent.loc.start.column === 0) { | ||
return; | ||
} | ||
if ( | ||
isPairWithKey( | ||
node.parent.parent.parent.parent?.parent?.parent?.parent ?? null, | ||
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 there any way to prevent that kind of path ? 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 a very specific edge case that we want to solve here, it won't be used anywhere else but if we need to we can have a function that checks N parents above, I will add it if I ever encounter a parent chain I will add it ! 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. We can maybe have a function that will find the deepest parent in a given element, so it handles even deeper ones etc. 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. Here it's not necessarily the deepest, it's just that's the 7th parent has be to a pair with key 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. Yes but there should only be one 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. we could, but that won't make the code simpler so I don't see why change 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 was more to make sure if it's the 6th or 9th it won't require an other manual check, it's not necessary to change it right now, we can iterate on it if there's an issue one day |
||
'servers' | ||
) | ||
) { | ||
// accept out of line enum if in servers | ||
return; | ||
} | ||
context.report({ | ||
node: node.parent.parent as any, | ||
messageId: 'enumNotOutOfLine', | ||
}); | ||
}, | ||
}; | ||
}, | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,55 @@ | ||
import type { Rule } from 'eslint'; | ||
|
||
import { isBLockScalar, isPairWithKey, isScalar } from '../utils'; | ||
|
||
export const singleQuoteRef: Rule.RuleModule = { | ||
meta: { | ||
docs: { | ||
description: '$ref must be wrapped in single quote', | ||
}, | ||
messages: { | ||
refNoQuote: '$ref is not wrapped in single quote', | ||
}, | ||
fixable: 'code', | ||
}, | ||
create(context) { | ||
if (!context.parserServices.isYAML) { | ||
return {}; | ||
} | ||
|
||
return { | ||
YAMLPair(node): void { | ||
if (!isPairWithKey(node, '$ref')) { | ||
return; | ||
} | ||
if (!isScalar(node.value)) { | ||
// not our problem, something else will fail like path resolution | ||
return; | ||
} | ||
if (node.value.style === 'single-quoted') { | ||
// that's what we want | ||
return; | ||
} | ||
if (isBLockScalar(node.value)) { | ||
damcou marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// another rule should take care of that case | ||
return; | ||
} | ||
const hasDoubleQuote = node.value.style === 'double-quoted'; | ||
const [start, end] = node.value.range; | ||
context.report({ | ||
node: node as any, | ||
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. how come we need to cast 🤔 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. There is an issue between eslint type |
||
messageId: 'refNoQuote', | ||
*fix(fixer) { | ||
if (hasDoubleQuote) { | ||
yield fixer.replaceTextRange([start, start + 1], "'"); | ||
yield fixer.replaceTextRange([end - 1, end], "'"); | ||
} else { | ||
yield fixer.insertTextBeforeRange([start, start], "'"); | ||
yield fixer.insertTextAfterRange([end, end], "'"); | ||
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'd check if the last character is not 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 sure if I saw it elsewhere but we also need to make sure the string does not contain a 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. pretty sure 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 yes this is only for |
||
} | ||
}, | ||
}); | ||
}, | ||
}; | ||
}, | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,66 @@ | ||
import { RuleTester } from 'eslint'; | ||
|
||
import { outOfLineEnum } from '../src/rules/outOfLineEnum'; | ||
|
||
const ruleTester = new RuleTester({ | ||
parser: require.resolve('yaml-eslint-parser'), | ||
}); | ||
|
||
ruleTester.run('out-of-line-enum', outOfLineEnum, { | ||
valid: [ | ||
` | ||
simple: | ||
type: string | ||
enum: [bla, blabla] | ||
`, | ||
` | ||
simple: | ||
type: string | ||
enum: | ||
- bla | ||
- blabla | ||
`, | ||
` | ||
simple: | ||
type: string | ||
enum: [bla, blabla] | ||
|
||
useIt: | ||
$ref: '#/simple' | ||
`, | ||
` | ||
servers: | ||
- url: http://test-server.com | ||
variables: | ||
region: | ||
default: us | ||
enum: | ||
- us | ||
- de | ||
`, | ||
], | ||
invalid: [ | ||
{ | ||
code: ` | ||
root: | ||
inside: | ||
type: string | ||
enum: [bla, blabla] | ||
`, | ||
errors: [{ messageId: 'enumNotOutOfLine' }], | ||
}, | ||
{ | ||
code: ` | ||
root: | ||
inside: | ||
deeper: | ||
type: string | ||
enum: [bla, blabla] | ||
|
||
useIt: | ||
$ref: '#/root/inside/deeper' | ||
`, | ||
errors: [{ messageId: 'enumNotOutOfLine' }], | ||
}, | ||
], | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,82 @@ | ||
import { RuleTester } from 'eslint'; | ||
|
||
import { singleQuoteRef } from '../src/rules/singleQuoteRef'; | ||
|
||
const ruleTester = new RuleTester({ | ||
parser: require.resolve('yaml-eslint-parser'), | ||
}); | ||
|
||
ruleTester.run('single-quote-ref', singleQuoteRef, { | ||
valid: [ | ||
` | ||
simple: | ||
$ref: 'random/path.yml'`, | ||
` | ||
sameFile: | ||
$ref: '#/inside'`, | ||
` | ||
long: | ||
$ref: 'some/random/file.yml#/root/object/prop' | ||
`, | ||
` | ||
post: | ||
description: test desc. | ||
'400': | ||
$ref: '../../common/responses/BadRequest.yml' | ||
`, | ||
], | ||
invalid: [ | ||
{ | ||
code: ` | ||
simple: | ||
$ref: random/path.yml | ||
`, | ||
errors: [{ messageId: 'refNoQuote' }], | ||
output: ` | ||
simple: | ||
$ref: 'random/path.yml' | ||
`, | ||
}, | ||
{ | ||
code: ` | ||
long: | ||
$ref: some/random/file.yml#/root/object/prop | ||
`, | ||
errors: [{ messageId: 'refNoQuote' }], | ||
output: ` | ||
long: | ||
$ref: 'some/random/file.yml#/root/object/prop' | ||
`, | ||
}, | ||
{ | ||
code: ` | ||
post: | ||
description: test desc. | ||
'400': | ||
$ref: ../../common/responses/BadRequest.yml | ||
'404': | ||
$ref: ../../common/responses/IndexNotFound.yml | ||
`, | ||
errors: [{ messageId: 'refNoQuote' }, { messageId: 'refNoQuote' }], | ||
output: ` | ||
post: | ||
description: test desc. | ||
'400': | ||
$ref: '../../common/responses/BadRequest.yml' | ||
'404': | ||
$ref: '../../common/responses/IndexNotFound.yml' | ||
`, | ||
}, | ||
{ | ||
code: ` | ||
double: | ||
$ref: "some/ref" | ||
`, | ||
errors: [{ messageId: 'refNoQuote' }], | ||
output: ` | ||
double: | ||
$ref: 'some/ref' | ||
`, | ||
}, | ||
], | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -41,6 +41,39 @@ async function propagateTagsToOperations( | |
return true; | ||
} | ||
|
||
async function lintCommon(verbose: boolean, useCache: boolean): Promise<void> { | ||
let hash = ''; | ||
const cacheFile = toAbsolutePath(`specs/dist/common.cache`); | ||
if (useCache) { | ||
const { cacheExists, hash: newCache } = await checkForCache( | ||
{ | ||
job: 'common specs', | ||
folder: toAbsolutePath('specs/'), | ||
generatedFiles: [], | ||
filesToCache: ['common'], | ||
cacheFile, | ||
}, | ||
verbose | ||
); | ||
|
||
if (cacheExists) { | ||
return; | ||
} | ||
|
||
hash = newCache; | ||
} | ||
|
||
const spinner = createSpinner('linting common spec', verbose).start(); | ||
await run(`yarn specs:lint common`, { verbose }); | ||
Comment on lines
+66
to
+67
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. don't we want to fix when it's not the CI? 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 a fan of scripts modifying hand written file when it's unclear, but here it should be fine 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. Make sense but if it's only for dev purposes it's fine I guess (as you want) |
||
|
||
if (hash) { | ||
spinner.text = `storing common spec cache`; | ||
await fsp.writeFile(cacheFile, hash); | ||
} | ||
|
||
spinner.succeed(); | ||
} | ||
|
||
async function buildSpec( | ||
client: string, | ||
outputFormat: string, | ||
|
@@ -87,7 +120,7 @@ async function buildSpec( | |
} | ||
|
||
spinner.text = `linting ${client} spec`; | ||
await run(`yarn specs:lint ${client}`, { verbose }); | ||
await run(`yarn specs:fix ${client}`, { verbose }); | ||
|
||
spinner.text = `validating ${client} spec`; | ||
await run(`yarn openapi lint specs/bundled/${client}.${outputFormat}`, { | ||
|
@@ -113,6 +146,8 @@ export async function buildSpecs( | |
): Promise<void> { | ||
await fsp.mkdir(toAbsolutePath('specs/dist'), { recursive: true }); | ||
|
||
await lintCommon(verbose, useCache); | ||
|
||
await Promise.all( | ||
clients.map((client) => buildSpec(client, outputFormat, verbose, useCache)) | ||
); | ||
|
Uh oh!
There was an error while loading. Please reload this page.