Fix many ESlint errors #1265
Fix many ESlint errors #1265
Conversation
For mysterious reasons, these errors appear on my branch pyscript#1262 even though they are not related to changes there. The eslint config seems a bit unstable. Anyways this fixes them.
|
I'm really confused about the behavior of eslint. Locally I see quite different output... |
|
I wouldn't swear it's the same issue, but we had a weird thing happen with ESLint back in the fall, where a particular objectionable line was 'warning' locally (when running in desktop Ubuntu 22.04) but 'erroring' in CI (on GitHub's docker image of Ubuntu 22.04). Didn't ever get to the bottom of exactly what the difference was, but running that docker image locally allowed for more visibility into the differences. |
|
Thanks for the info @JeffersGlass, is the github actions ubuntu 22.04 image available on docker hub? I'm having trouble finding it. |
|
Though I guess that's a useless direction because we are running it on pre-commit.ci and not on github actions. |
|
👏 👏 👏 👏 👏 |
|
Still using toml-j0.4? |
Yes. I'm not sure what the previous investigation into toml libraries was, but currently as far as I can tell there are four main options:
There are a bunch of other options with <1k weekly downloads -- some of these look promising but it's better to stick with popular libs. I think @ltd/j-toml is the best for us followed by toml-j0.4. If we decide to switch to @ltd/j-toml we can do it in a followup. |
|
Could someone merge this? I think it's ready. |
|
We looked at different TOML libs and evaluated all these...
<img width="1569" alt="Screen Shot 2023-03-09 at 7 53 57 AM" src="https://user-images.githubusercontent.com/35942272/224048126-2ca47112-d41a-46e3-9d06-c9d91d3b9e3f.png">
After fast-toml had some error issues, we switched to toml-j0.4. 0.4.0 is
actually identical to TOML 0.5.0 spec wise. I have a preference for small
dead libraries, they do not change and break things, unless we change them.
All good.
…On Thu, Mar 9, 2023 at 7:54 AM Hood Chatham ***@***.***> wrote:
Could someone merge this? I think it's ready.
—
Reply to this email directly, view it on GitHub
<#1265 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AISG7ADBQVCAOB2IIJTFHA3W3HOIPANCNFSM6AAAAAAVTWITX4>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
|
fast-toml also has alarmingly few users compared to the other options listed... |
|
One problem with toml-j0.4 is that it has type definitions in On the other hand, @ltd/j-toml and toml both correctly bundle type definitions. |
pyscriptjs/Makefile
Outdated
| # toml-j0.4 left out its types from the npm dist | ||
| # See https://github.com/jakwings/toml-j0.4/pull/8 | ||
| cd node_modules/toml-j0.4 && wget https://raw.githubusercontent.com/jakwings/toml-j0.4/v1.1.1/toml.d.ts |
There was a problem hiding this comment.
@tedpatrick workaround for toml-j0.4's missing types
| @@ -49,7 +49,7 @@ | |||
| "@codemirror/state": "6.1.2", | |||
| "@codemirror/theme-one-dark": "6.1.0", | |||
| "@codemirror/view": "6.3.0", | |||
| "codemirror": "6.0.1", | |||
| "toml-j0.4": "^1.1.1" | |||
| "@hoodmane/toml-j0.4": "^1.1.2", | |||
There was a problem hiding this comment.
@vyings merged my pull request adding the type definitions to the toml-j0.4 distribution, and tagged a new version v1.1.2. However he says he has lost access to the npm registry account so I published v1.1.2 of tom-j0.4 here. This way we don't need to wget to patch up the types.
For mysterious reasons, these errors appear on my branch #1262 even
though they are not related to changes there. The eslint config seems
a bit unstable.
Anyways this fixes them.