Skip to content
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

(Yet another) comment issue #27

Open
pgalbavy-itrs opened this issue Nov 10, 2022 · 1 comment
Open

(Yet another) comment issue #27

pgalbavy-itrs opened this issue Nov 10, 2022 · 1 comment

Comments

@pgalbavy-itrs
Copy link

Again, thanks for the rapid fix for the previous issue, but found yet another one. I can share our full default config file if it helps?

key: {
    # //
    name: {
        name: value
    }
}

Any URL in a comment before a { ... } encodes object causes an error - I will assume it's the '//' being seen as a comment as well and getting something out of sync. I have not checked what happens for /* ... */ inside # or // prefixed single lines, but I am guessing it's going to have similar issues...

(I have confirmed that # /* this is an embedded comment */ also triggers an error)

Originally posted by @pgalbavy-itrs in #26 (comment)

@josh-wolfe-okcupid
Copy link
Contributor

I took a look at solving this, and there's a pretty major problem where this package is using the golang tokenizer in the "test/scanner" package, and Go comments are skipped by default. This causes the bug in this issue by the "//\n" getting fully skipped and then the scanner looking for the end of the # comment consumes the following line too.

I have confirmed that this implementation of hocon incorrectly accepts /* ... */ style comments due to using the Golang tokenizer. I feel like fixing this would require fully replacing the scanner.Scanner with a custom tokenizer specifically for hocon, rather than trying to patch the Golang tokenizer. Given that this issue has been open nearly 2 years with no activity, I'm not inclined to do the work to contribute that major of an enhancement/redesign to this repo. i think it's gotta be the original maintainer that fixes this. I got this far before giving up.

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

No branches or pull requests

2 participants