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

json parser tries to parse nested toml #469

Closed
dennis-wey opened this issue May 13, 2024 · 4 comments
Closed

json parser tries to parse nested toml #469

dennis-wey opened this issue May 13, 2024 · 4 comments

Comments

@dennis-wey
Copy link

dennis-wey commented May 13, 2024

I think this is a bug from #113

I'm having trouble getting the new config options to work with pre-commit:

pre-commit-config.yaml

repos:
  - repo: "https://github.com/igorshubovych/markdownlint-cli"
    rev: "v0.40.0"
    hooks:
      - id: "markdownlint-fix"
        args: [--config, pyproject.toml, --configPointer, "/tool/markdownlint"]
        verbose: true

pyproject.toml

[project]
name = "my-package"
description = "description"

[tool.markdownlint]
default = true
MD007.indent = 4

I get the following error:

pre-commit run -a
markdownlint-fix.........................................................Failed
- hook id: markdownlint-fix
- duration: 0.13s
- exit code: 1

Cannot read or parse config file 'pyproject.toml': Unable to parse 'pyproject.toml'; Parser 0: Unable to parse JSON(C) content, InvalidSymbol (offset 1, length 7), InvalidSymbol (offset 10, length 4), InvalidSymbol (offset 15, length 1), EndOfFileExpected (offset 17, length 12); Parser 1: Expected "=", [ \t] or [A-Za-z0-9_\-] but "." found.; Parser 2: end of the stream or a document separator is expected (2:1)

 1 | [project]
 2 | name = "my-package"
-----^
 3 | description = "description"
 4 | 

Process finished with exit code 1

Seems like it tries to apply the JSON Parser first to the toml file.
After that the right configuration is found with the toml parser and the actual formatting is taking place but the prior error is causing the action to exit with status code 1.

This is especially problematic in ci workflows which rely on actions exit with status code 0 to pass

@DavidAnson
Copy link
Collaborator

When attempting to parse a configuration file, all three parsers are tried and the result of the first successful is used. In this case, all three are failing as shown in the error message above. The TOML parser is second in the array (index 1), so the following output is relevant:

Parser 1: Expected "=", [ \t] or [A-Za-z0-9_-] but "." found.

No line/column information is provided, but I think it is referring to the following key name as being invalid because it includes a dot: MD007.indent.

@dennis-wey
Copy link
Author

Hi @DavidAnson thanks for your answer. I tried a bit around with the setup and made some observations:

  • I can confirm, that it's complaining about MD007.indent = 4 line
  • the toml parser still somehow works. I can confirm that the remaining config is still used. Seems like it just skips over the not understandable lines

That said, the file example stated above seems to me like a valid toml format: https://toml.io/en/v1.0.0#table
While rewriting the config in the minimal example above would be no problem, the pyproject.toml for most users might be much bigger and there are use cases where avoiding this syntax is more annoying.
My other hooks (mypy, ruff) also reads the pyproject.toml and don't have problems with this kind of syntax.

So it seems to me there is either a bug in the used toml parser or the parser doesn't accept the whole toml spec.

@DavidAnson
Copy link
Collaborator

This is the TOML parser that is used now: https://www.npmjs.com/package/toml

It says it supports version 0.4.0 of the specification which I confirm prohibits dots in key names: https://toml.io/en/v0.4.0#table

If you know of another well-used, dependency-free TOML parser for Node that supports a more recent version, I can have a look at switching to that.

DavidAnson added a commit that referenced this issue May 15, 2024
…-toml that exports CommonJS instead of a Module (refs #469).
@dennis-wey
Copy link
Author

dennis-wey commented May 15, 2024

Hi @DavidAnson, I'm not a node/js developer so I also have no experience in that regard.

Searching through npm this one seems the most promising:
https://www.npmjs.com/package/smol-toml

  • supports toml 1.0.0
  • has recently released versions
  • no dependencies
  • seems to be used ( although not as many downloads as your current parser)

Edit: Seems like you came to the same conclusion a bit earlier 😄

DavidAnson added a commit that referenced this issue May 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants