Skip to content

fix(parser): return instead of throwing on an unterminated scope #55

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.

Sign up for GitHub

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

Open
svyatov wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(parser): return instead of throwing on an unterminated scope #55

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion lib/parser.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,9 @@ function summary (scanner) {
// ... "(" <scope> ")" ...
let s = scope(scanner)
if (s instanceof Error) {
// An opened scope that never closes is a syntax error in a <summary>, where
// the grammar demands the ")". Everywhere else it just means "no scope here".
if (s.unterminatedScope) return s
s = null
} else {
node.children.push(s)
Expand Down Expand Up @@ -157,6 +160,7 @@ function text (scanner) {
* "(" <scope> ")" ::= 1*<any UTF8-octets except newline or parens>
*/
function scope (scanner) {
const start = scanner.position()
if (scanner.peek() !== '(') {
return scanner.abort(scanner.enter('scope', ''))
} else {
Expand All @@ -174,7 +178,14 @@ function scope (scanner) {
}

if (scanner.peek() !== ')') {
throw scanner.abort(node, [')'])
// Return rather than throw, so a <body> line that merely looks like a scope
// ("foo(bar(1))" at the start of a line) can fall back to <text>. abort()
// rewinds only as far as the scope node, which starts after the "(", so
// rewind past the "(" too and leave the scanner where the caller found it.
const error = scanner.abort(node, [')'])
scanner.rewind(start)
error.unterminatedScope = true
return error
} else {
scanner.exit(node)
scanner.next()
Expand Down
29 changes: 29 additions & 0 deletions test/parser.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,30 @@ describe('<message>', () => {
const parsed = parser('feat: breaking change\n\nBREAKING CHANGE: introduces breaking change\nsecond line')
parsed.should.matchSnapshot()
})
it('treats a line starting with nested parens as <text>, not a <scope>', () => {
const parsed = parser('fix: address major bug\n\nfoo(bar(1))')
expect(bodyChildren(parsed)).to.deep.equal([['text', 'foo(bar(1))']])
})
it('treats a line starting with an unterminated paren as <text>', () => {
// https://github.com/conventional-commits/parser/pull/48
const parsed = parser('fix(foo): some renovate commit\n\nfoo(\n)')
expect(bodyChildren(parsed)).to.deep.equal([['text', 'foo('], ['newline', '\n'], ['text', ')']])
})
it('still parses footers after such a line', () => {
const parsed = parser('fix: address major bug\n\nz.array(z.boolean())\n\nRefs #392')
expect(bodyChildren(parsed)).to.deep.equal([['text', 'z.array(z.boolean())']])
expect(parsed.children.filter(c => c.type === 'footer')).to.have.lengthOf(1)
})
it('contains valid positions for a line starting with nested parens', () => {
assertNodePositions('fix: address major bug\n\nfoo(bar(1))')
})
it('still throws for nested parens in a <summary> scope', () => {
// <scope> ::= 1*<any UTF8-octets except newline or parens>, so this is
// a genuine syntax error, unlike the same characters in a <body>.
expect(() => {
parser('fix(a(b)): x')
}).to.throw("unexpected token '(' at 1:6, valid tokens [)]")
})
})
describe('<body>, <newline>*, <footer>+', () => {
it('parses footer after body', () => {
Expand All @@ -133,6 +157,11 @@ describe('<message>', () => {
})
})

function bodyChildren (parsed) {
const body = parsed.children.find(node => node.type === 'body')
return body ? body.children.map(node => [node.type, node.value]) : []
}

function assertNodePositions (text) {
visit(parser(text), node => {
if (node.children) return
Expand Down