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

Cannot load configuration file: invalid type: integer 200, expected a string #1338

Closed
nvuillam opened this issue Jan 7, 2024 · 7 comments · Fixed by #1345
Closed

Cannot load configuration file: invalid type: integer 200, expected a string #1338

nvuillam opened this issue Jan 7, 2024 · 7 comments · Fixed by #1345

Comments

@nvuillam
Copy link

nvuillam commented Jan 7, 2024

Example in documentation contains the following example:

image

But when using the latest version, we have the following error:

 Error:  Error while loading config: Cannot load configuration file `/github/workspace/.github/linters/lychee.toml`: Failed to parse configuration file
  
  Caused by:
      TOML parse error at line 32, column 11
         |
      32 | accept = [200, 206, 429]
         |           ^^^
      invalid type: integer `200`, expected a string

Out lychee.toml file contains:

# Comma-separated list of accepted status codes for valid links.
accept = [200, 206, 429]
nvuillam pushed a commit to oxsecurity/megalinter that referenced this issue Jan 7, 2024
nvuillam pushed a commit to oxsecurity/megalinter that referenced this issue Jan 7, 2024
Upgrade lychee default configuration to handle [breaking change between 0.13.0 and 0.14.0](lycheeverse/lychee#1338)
nvuillam added a commit to oxsecurity/megalinter that referenced this issue Jan 7, 2024
* [automation] Auto-update linters version, help and documentation

* Fix lychee breaking change

Upgrade lychee default configuration to handle [breaking change between 0.13.0 and 0.14.0](lycheeverse/lychee#1338)

* Undowngrade revive

* Fix lychee config until bug is solved

lycheeverse/lychee#1340

* [MegaLinter] Apply linters fixes

---------

Co-authored-by: nvuillam <nicolas.vuillamy@cloudity.com>
Co-authored-by: nvuillam <nvuillam@users.noreply.github.com>
@mre
Copy link
Member

mre commented Jan 7, 2024

Oh, okay. We changed the parsing and status codes need to be strings now. The reason is that ranges are also supported now, e.g. "200..=204". Range syntax is identical to the normal Rust range syntax. We should update the docs to reflect that. Want to send a pull request?
Maybe we could support both, strings and integers, but I don't know how easy that will be.

@nvuillam
Copy link
Author

nvuillam commented Jan 7, 2024

@mre I think the best would be to support both in a 0.14.1 patch, because if after a version upgrade, existing configuration file triggers a crash, it means it's a breaking change, and according to semver, in that case 0.14.0 should have been 1.0.0 :)

@mre
Copy link
Member

mre commented Jan 7, 2024

Supporting both is probably ideal yes.

Just would like to make one small remark that semver allows breaking changes before 1.0. In fact, that's the very reason why lychee is not 1.0 yet: I do anticipate quite a few more breaking changes before the first stable release; it's just that this specific one is probably avoidable. Whether we will support both, string and int, depends on the complexity of the implementation and what side effects this will have down the road. So far I don't see any reason why it shouldn't be possible.

@nvuillam
Copy link
Author

nvuillam commented Jan 7, 2024

Indeed you're right about semver, I forgot the "before 1.0.0 rule" :)

But as lychee is more and more popular (it has been embedded in MegaLinter by a contributor from Microsoft using it on MS internal repos ^^) , projects are more and more dependent on it, maybe it would be time to release a 1.0.0 ? 👼

@Techassi
Copy link
Contributor

Techassi commented Jan 8, 2024

We should be able to enable support for single integers and list of integers by widening our custom serde implementation. I could tackle this is a PR in the next few days.

@Techassi
Copy link
Contributor

Techassi commented Jan 9, 2024

This will be fixed as soon as #1345 is merged.

@mre
Copy link
Member

mre commented Jan 9, 2024

Amazing work!

@mre mre closed this as completed in #1345 Jan 9, 2024
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 a pull request may close this issue.

3 participants