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

fixed eslint and linted files #8891

Closed
wants to merge 1 commit into from
Closed

Conversation

wvanderp
Copy link
Contributor

Hello,

In this PR, I fixed eslint because it breaks on the new assert {type: 'json'} syntax. I fixed it by introducing Babel to interpret the syntax. I then solved the lining issues that arose.

There is a lot to like in this PR. But there are a few controversial decisions I made.

Why do we need Babel?
eslint/eslint#15305 states that eslint will only be implemented when the TC accepts it, so we need Babel as a temporary stopgap.

Why did you remove the commented-out code?
there were a lot of unused variables and imports that were only there for the unused code. So that either needed to be commented out or needed to be removed. I disliked "dead" code because it does not get updated, keeping it in a working state code-wise and idea-wise.

You switched the fork of json-stringify-pretty-compact. Why?
I saw that typescript was complaining about the import. That is why I started looking at what was happening, and I saw that this was a fork of another repo. I don't know the original reason for changing, but the original repo started getting updates again. So I switched back, and that fixed the problem and introduced no new regressions.

You added lining to the GitHub actions. This means that people who want to update the index will need to fix the lining errors in the glue code.
Because we now get a notification when there is a linting error, we can let the person who introduced it fix it.

Why is cSpell now included in .vscode?
cSpell is the most used spellchecker for vscode. So, capturing some OSM-specific words will help prevent error fatigue.

I am open to feedback and applying any changes you feel necessary.
-wvdp

@bhousel
Copy link
Member

bhousel commented Oct 29, 2023

Thanks for this, #6014 has some of the history from when the new import assert was introduced by node, and why eslint is disabled.

Sorry to say I am uncomfortable accepting this PR, and would prefer to decline it. I'm pretty ok leaving eslint disabled for now, and not comfortable introducing a dependency on babel just for this.

Some of the other changes might make sense (like switching the json prettier) - we should open issues for these things if there is value there.

I don't understand the VSCode changes, and I don't use VSCode personally. I don't understand why anyone would want spell checking in a code editor.

@bhousel bhousel closed this Oct 29, 2023
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.

2 participants