Skip to content

Fails to parse when body includes something that looks like a scope with new line#48

Open
Otard95 wants to merge 3 commits into
conventional-commits:mainfrom
Otard95:fix/scope-in-body
Open

Fails to parse when body includes something that looks like a scope with new line#48
Otard95 wants to merge 3 commits into
conventional-commits:mainfrom
Otard95:fix/scope-in-body

Conversation

@Otard95

@Otard95 Otard95 commented Mar 18, 2024

Copy link
Copy Markdown

I originally came across this when release-please didn't pick up one of our commits. Eventually I tracked it down to this package. I'm creating a PR, mostly to provide the failing test showing the issue. I minimized the test as much as possible, here's a snippet of the original text from the commit that failed to parse, if that's helpful:

    ```graphql
@​link(url: "https://specs.apollo.dev/federation/v2.6", import:
[..., "@​policy"])
    ```

The code block is a result of the text in this PR apollographql/federation#2818 ending up in one of our Renovate PR's and subsequently commit body.

@Otard95 Otard95 changed the title Fails to parse when body includes something that looks like a scope Fails to parse when body includes something that looks like a scope with new line Mar 18, 2024

@wesleytodd wesleytodd left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This approval is just to kick the CI into action. If this doesn't do the trick I will see about pulling this down to run locally.

EDIT: yep, committing that white space removal was enough to make the "approve and run actions" button show up.

Comment thread test/parser.js Outdated
maybe removing the whitespace here will kick off the CI.

@wesleytodd wesleytodd left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing to request changes since this was failing in CI.

@svyatov

svyatov commented Jul 21, 2026

Copy link
Copy Markdown

Your case is fixed in #55, which I just opened. scope() returns the Error instead of throwing it, so a line that only looks like a scope falls back to <text> and the commit parses.

I lifted your assertions close to verbatim, since foo(\n) is exactly the shape that breaks, and credited you in the PR. Thanks for pinning it down two years before I hit it.

The same change covers #54, the nested-paren version of the same bug, where a line like z.array(z.boolean()) at column 0 does it. That is how it reached me: release-please catches the throw and silently drops the entire commit from the changelog, so we lost an entry from two consecutive release PRs before anyone noticed it was gone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants