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
aio + docs/api: implement content checks #22759
Closed
petebacondarwin
wants to merge
8
commits into
angular:master
from
petebacondarwin:aio-api-check-headings
Closed
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
bd395c0
build(aio): add checkContentRules processor
petebacondarwin 9447e94
build(aio): create noMarkdownHeadings content rule
petebacondarwin 87d950a
build(aio): implement rules to prevent headings in content
petebacondarwin 29afa57
build(aio): create minLength content rule
petebacondarwin c8c3233
build(aio): implement rules to prevent short parameter names
petebacondarwin f5f3c51
fixup! build(aio): create noMarkdownHeadings content rule
petebacondarwin 97b8fec
fixup! build(aio): create noMarkdownHeadings content rule
petebacondarwin d64e868
fixup! build(aio): add checkContentRules processor
petebacondarwin File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
8 changes: 8 additions & 0 deletions
8
aio/tools/transforms/angular-api-package/content-rules/minLength.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
module.exports = function createMinLengthRule(minLength) { | ||
minLength = minLength || 2; | ||
return (doc, prop, value) => { | ||
if (value.length < minLength) { | ||
return `Invalid "${prop}" property: "${value}". It must have at least ${minLength} characters.`; | ||
} | ||
}; | ||
}; |
19 changes: 19 additions & 0 deletions
19
aio/tools/transforms/angular-api-package/content-rules/minLength.spec.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
const createMinLengthRule = require('./minLength'); | ||
|
||
describe('createMinLength rule', () => { | ||
|
||
const defaultRule = createMinLengthRule(); | ||
const atLeast5CharsRule = createMinLengthRule(5); | ||
|
||
it('should return `undefined` if the length of the property value is long enough', () => { | ||
expect(defaultRule({}, 'description', 'abc')).toBeUndefined(); | ||
expect(atLeast5CharsRule({}, 'description', 'abcde')).toBeUndefined(); | ||
}); | ||
|
||
it('should return an error message if length of the property value is not long enough', () => { | ||
expect(defaultRule({}, 'description', 'a')) | ||
.toEqual('Invalid "description" property: "a". It must have at least 2 characters.'); | ||
expect(atLeast5CharsRule({}, 'description', 'abcd')) | ||
.toEqual('Invalid "description" property: "abcd". It must have at least 5 characters.'); | ||
}); | ||
}); |
50 changes: 50 additions & 0 deletions
50
aio/tools/transforms/angular-api-package/content-rules/noMarkdownHeadings.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,50 @@ | ||
/** | ||
* A factory for creating a rule for the `checkContentRules` processor, which disallows markdown | ||
* headings in a content property. | ||
* | ||
* @param {...number|string} disallowedHeadings | ||
* Each parameter identifies heading levels that are not allowed. They can be in the form of: | ||
* | ||
* - a number (e.g. 1), which implies that the specified heading is not allowed | ||
* - a range (e.g. '2,3'), which implies the range of headings that are not allowed | ||
* | ||
* (A range can be open ended on the upper bound by not specifying a value after the comma.) | ||
* | ||
* @example | ||
* To create a rule that will only allow level 3 headings: | ||
* | ||
* ``` | ||
* const rule = createNoMarkdownHeadingRule(1, 2, '4,'); | ||
* ``` | ||
* | ||
*/ | ||
module.exports = function createrNoMarkdownHeadingRule() { | ||
const args = Array.prototype.slice.apply(arguments); | ||
const disallowedHeadings = args.map(arg => `#{${arg}}`); | ||
if (!disallowedHeadings.length) { | ||
disallowedHeadings.push('#{1,}'); | ||
} | ||
const regex = new RegExp(`^ {0,3}(${disallowedHeadings.join('|')}) +.*$`, 'mg'); | ||
return (doc, prop, value) => { | ||
let match, matches = []; | ||
while(match = regex.exec(value)) { // eslint-disable-line no-cond-assign | ||
matches.push(match[0]); | ||
} | ||
if (matches.length) { | ||
const list = listify(matches.map(match => `"${match}"`)); | ||
return `Invalid headings found in "${prop}" property: ${list}.`; | ||
} | ||
}; | ||
}; | ||
|
||
|
||
/** | ||
* Convert an array of strings in to a human list - e.g separated by commas and the word `and`. | ||
* @param {string[]} values The strings to convert to a list | ||
*/ | ||
function listify(values) { | ||
if (values.length <= 1) return values; | ||
const last = values[values.length - 1]; | ||
const rest = values.slice(0, values.length - 1); | ||
return [rest.join(', '), last].join(' and '); | ||
} |
59 changes: 59 additions & 0 deletions
59
aio/tools/transforms/angular-api-package/content-rules/noMarkdownHeadings.spec.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,59 @@ | ||
const createNoMarkdownHeadings = require('./noMarkdownHeadings'); | ||
|
||
describe('createNoMarkdownHeadings rule', () => { | ||
|
||
const noMarkdownHeadings = createNoMarkdownHeadings(); | ||
|
||
it('should return `undefined` if there is no heading in a value', () => { | ||
expect(noMarkdownHeadings({}, 'description', 'some ## text')) | ||
.toBeUndefined(); | ||
}); | ||
|
||
it('should return an error message if there is a markdown heading in a single line value', () => { | ||
expect(noMarkdownHeadings({}, 'description', '# heading 1')) | ||
.toEqual('Invalid headings found in "description" property: "# heading 1".'); | ||
}); | ||
|
||
it('should return an error message if there is a markdown heading in a multiline value', () => { | ||
expect(noMarkdownHeadings({}, 'description', 'some text\n# heading 1')) | ||
.toEqual('Invalid headings found in "description" property: "# heading 1".'); | ||
}); | ||
|
||
it('should cope with up to 3 spaces before the heading marker', () => { | ||
expect(noMarkdownHeadings({}, 'description', ' # heading 1')) | ||
.toEqual('Invalid headings found in "description" property: " # heading 1".'); | ||
expect(noMarkdownHeadings({}, 'description', ' # heading 1')) | ||
.toEqual('Invalid headings found in "description" property: " # heading 1".'); | ||
expect(noMarkdownHeadings({}, 'description', ' # heading 1')) | ||
.toEqual('Invalid headings found in "description" property: " # heading 1".'); | ||
}); | ||
|
||
it('should return an error message for each heading found', () => { | ||
expect(noMarkdownHeadings({}, 'description', '# heading 1\nsome text\n## heading 2\nmore text\n### heading 3')) | ||
.toEqual('Invalid headings found in "description" property: "# heading 1", "## heading 2" and "### heading 3".'); | ||
}); | ||
|
||
describe('(specified heading levels)', () => { | ||
it('should take heading levels into account', () => { | ||
const noTopLevelHeadings = createNoMarkdownHeadings(1); | ||
expect(noTopLevelHeadings({}, 'description', '# top level')) | ||
.toEqual('Invalid headings found in "description" property: "# top level".'); | ||
expect(noTopLevelHeadings({}, 'description', '## second level')) | ||
.toBeUndefined(); | ||
expect(noTopLevelHeadings({}, 'description', '### third level')) | ||
.toBeUndefined(); | ||
expect(noTopLevelHeadings({}, 'description', '#### fourth level')) | ||
.toBeUndefined(); | ||
|
||
const allowLevel3Headings = createNoMarkdownHeadings(1, 2, '4,'); | ||
expect(allowLevel3Headings({}, 'description', '# top level')) | ||
.toEqual('Invalid headings found in "description" property: "# top level".'); | ||
expect(allowLevel3Headings({}, 'description', '## second level')) | ||
.toEqual('Invalid headings found in "description" property: "## second level".'); | ||
expect(allowLevel3Headings({}, 'description', '### third level')) | ||
.toBeUndefined(); | ||
expect(allowLevel3Headings({}, 'description', '#### fourth level')) | ||
.toEqual('Invalid headings found in "description" property: "#### fourth level".'); | ||
}); | ||
}); | ||
}); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
69 changes: 69 additions & 0 deletions
69
aio/tools/transforms/angular-base-package/processors/checkContentRules.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,69 @@ | ||
|
||
/** | ||
* A processor that can run arbitrary checking rules against properties of documents | ||
|
||
* The configuration for the processor is via the `docTypeRules`. | ||
* This is a hash of docTypes to rulesets. | ||
* Each rules set is a hash of properties to rule functions. | ||
* | ||
* The processor will run each rule function against each matching property of each matching doc. | ||
* | ||
* An example rule might look like: | ||
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. "A totally made-up example..." 😛 |
||
* | ||
* ``` | ||
* function noMarkdownHeadings(doc, prop, value) { | ||
* const match = /^\s?#+\s+.*$/m.exec(value); | ||
* if (match) { | ||
* return `Headings not allowed in "${prop}" property. Found "${match[0]}"`; | ||
* } | ||
* } | ||
* ``` | ||
* | ||
*/ | ||
module.exports = function checkContentRules(log, createDocMessage) { | ||
return { | ||
/** | ||
* { | ||
* [docType]: { | ||
* [property]: Array<(doc: Document, property: string, value: any) => string|undefined> | ||
* } | ||
* } | ||
*/ | ||
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. 💯 |
||
docTypeRules: {}, | ||
failOnContentErrors: false, | ||
$runAfter: ['tags-extracted'], | ||
$runBefore: ['processing-docs'], | ||
$process(docs) { | ||
const errors = []; | ||
docs.forEach(doc => { | ||
const docErrors = []; | ||
const rules = this.docTypeRules[doc.docType] || {}; | ||
if (rules) { | ||
Object.keys(rules).forEach(property => { | ||
const ruleFns = rules[property]; | ||
ruleFns.forEach(ruleFn => { | ||
const error = ruleFn(doc, property, doc[property]); | ||
if (error) { | ||
docErrors.push(error); | ||
} | ||
}); | ||
}); | ||
} | ||
if (docErrors.length) { | ||
errors.push({ doc, errors: docErrors }); | ||
} | ||
}); | ||
|
||
if (errors.length) { | ||
log.error('Content contains errors'); | ||
errors.forEach(docError => { | ||
const errors = docError.errors.join('\n '); | ||
log.error(createDocMessage(errors + '\n ', docError.doc)); | ||
}); | ||
if (this.failOnContentErrors) { | ||
throw new Error('Stopping due to content errors.'); | ||
} | ||
} | ||
} | ||
}; | ||
}; |
108 changes: 108 additions & 0 deletions
108
aio/tools/transforms/angular-base-package/processors/checkContentRules.spec.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,108 @@ | ||
var testPackage = require('../../helpers/test-package'); | ||
var Dgeni = require('dgeni'); | ||
|
||
describe('checkContentRules processor', function() { | ||
let processor, logger; | ||
|
||
beforeEach(function() { | ||
const dgeni = new Dgeni([testPackage('angular-base-package')]); | ||
const injector = dgeni.configureInjector(); | ||
processor = injector.get('checkContentRules'); | ||
logger = injector.get('log'); | ||
}); | ||
|
||
it('should exist on the injector', () => { | ||
expect(processor).toBeDefined(); | ||
expect(processor.$process).toEqual(jasmine.any(Function)); | ||
}); | ||
|
||
it('shpuld run at the right time', () => { | ||
expect(processor.$runAfter).toEqual(['tags-extracted']); | ||
expect(processor.$runBefore).toEqual(['processing-docs']); | ||
}); | ||
|
||
it('should do nothing if not configured', () => { | ||
const docs = [{ docType: 'test', description: '## heading 2' }]; | ||
processor.$process(docs); | ||
expect(docs).toEqual([{ docType: 'test', description: '## heading 2' }]); | ||
|
||
expect(logger.error).not.toHaveBeenCalled(); | ||
}); | ||
|
||
it('should run configured rules against matching docs', () => { | ||
const nameSpy1 = jasmine.createSpy('name 1'); | ||
const nameSpy2 = jasmine.createSpy('name 2'); | ||
const nameSpy3 = jasmine.createSpy('name 3'); | ||
const descriptionSpy1 = jasmine.createSpy('description 1'); | ||
const descriptionSpy2 = jasmine.createSpy('description 2'); | ||
const descriptionSpy3 = jasmine.createSpy('description 3'); | ||
|
||
processor.docTypeRules = { | ||
'test1': { | ||
name: [nameSpy1, nameSpy3], | ||
description: [descriptionSpy1, descriptionSpy3] | ||
}, | ||
'test2': { | ||
name: [nameSpy2], | ||
description: [descriptionSpy2] | ||
} | ||
}; | ||
|
||
const docs = [ | ||
{ docType: 'test1', description: 'test doc 1', name: 'test-1' }, | ||
{ docType: 'test2', description: 'test doc 2', name: 'test-2' } | ||
]; | ||
processor.$process(docs); | ||
expect(nameSpy1).toHaveBeenCalledTimes(1); | ||
expect(nameSpy1).toHaveBeenCalledWith(docs[0], 'name', 'test-1'); | ||
expect(nameSpy2).toHaveBeenCalledTimes(1); | ||
expect(nameSpy2).toHaveBeenCalledWith(docs[1], 'name', 'test-2'); | ||
expect(nameSpy3).toHaveBeenCalledTimes(1); | ||
expect(nameSpy3).toHaveBeenCalledWith(docs[0], 'name', 'test-1'); | ||
expect(descriptionSpy1).toHaveBeenCalledTimes(1); | ||
expect(descriptionSpy1).toHaveBeenCalledWith(docs[0], 'description', 'test doc 1'); | ||
expect(descriptionSpy2).toHaveBeenCalledTimes(1); | ||
expect(descriptionSpy2).toHaveBeenCalledWith(docs[1], 'description', 'test doc 2'); | ||
expect(descriptionSpy3).toHaveBeenCalledTimes(1); | ||
expect(descriptionSpy3).toHaveBeenCalledWith(docs[0], 'description', 'test doc 1'); | ||
}); | ||
|
||
it('should log errors if the rule returns error messages', () => { | ||
const nameSpy1 = jasmine.createSpy('name 1').and.returnValue('name error message'); | ||
const descriptionSpy1 = jasmine.createSpy('description 1').and.returnValue('description error message'); | ||
|
||
processor.docTypeRules = { | ||
'test1': { | ||
name: [nameSpy1], | ||
description: [descriptionSpy1] | ||
} | ||
}; | ||
|
||
const docs = [ | ||
{ docType: 'test1', description: 'test doc 1', name: 'test-1' }, | ||
{ docType: 'test2', description: 'test doc 2', name: 'test-2' } | ||
]; | ||
|
||
processor.$process(docs); | ||
|
||
expect(logger.error).toHaveBeenCalledTimes(2); | ||
expect(logger.error).toHaveBeenCalledWith('Content contains errors'); | ||
expect(logger.error).toHaveBeenCalledWith(`name error message | ||
description error message | ||
- doc "test-1" (test1) `); | ||
}); | ||
|
||
it('should throw an error if `failOnContentErrors` is true and errors are found', () => { | ||
const errorRule = jasmine.createSpy('error rule').and.returnValue('some error'); | ||
processor.docTypeRules = { | ||
'test': { description: [errorRule] } | ||
}; | ||
processor.failOnContentErrors = true; | ||
|
||
const docs = [ | ||
{ docType: 'test', description: 'test doc' }, | ||
]; | ||
expect(() => processor.$process(docs)).toThrowError('Stopping due to content errors.'); | ||
}); | ||
|
||
}); |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
we should consider being a bit more aggressive and require at least 3 or 4 characters. I can't think of any variables that should be less than 4 chars, but we should test and see. I suggest we start with 4 and if we find legit usecases for variable names of length 3 then we can decrease it.
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.
How about
url
:-P This occurs "lots" of times.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.
Others that occur in the current code:
p
req
fn
key
k
v
ctx
obj
id
el
cd
dir
c
cb
max
min
res
err
cdr
sw
I would say that setting the minimum size to 3 chars would be a reasonable compromise.