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

fix: integrate dev tools and fix crusial errors and warnings #295

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

phts
Copy link
Contributor

@phts phts commented Jan 29, 2024

Integrated for whole repo, but enabled only for "spotify". Feel free to enable it for other repos in future in .eslintignore and .prettierignore files.

Easier to review commit-by-commit, which represent independent step

Commit 3024fce contains fixes of crucial errors which were found by eslint (which caused crashes and misbehavior)

@phts phts mentioned this pull request Jan 29, 2024
@volumio
Copy link
Owner

volumio commented Jan 30, 2024

Thanks for this, but I see that many global variables are removed (like selectedBitrate). This huge diff on index.js requires careful testing and it's too much for me to read and validate without very extensive testing.

IMHO:

  • Let's just lint and check everything besides index.js
  • On index, let's just fix errors (like done in 3024fce)

Otherwise it's really hard for me to check and get it in

@phts
Copy link
Contributor Author

phts commented Jan 31, 2024

Those globals are not used and not referenced anyhow. But I revered them

No sense to integrate without any automation, and keeping everything unchanged. Based on my experience, in this case it will never be fixed.

All those formatting and recommended codestyle is performed automatically on every commit. And that makes code style consistent for all contributors and prevent most typos and mistakes. This autofix is save and used by millions devs every day.

I couldn't fix those crucial mistakes without autofix by eslint all autofixable errors. Otherwise whole file would be red in my IDE, or I would to turn off all basic rules to ignore such warnings, which also makes whole eslint stuff almost useless.

Additionally I fixed warnings (like unused variable) manually and also is safe.

And better to review this commit-by-commit, those describe single independent change each other.

Usually, you can rebase all your future development onto this branch, and you will use all benefits of linting and test everything during development.

@phts phts force-pushed the eslint branch 3 times, most recently from facb37f to 6f7b543 Compare February 9, 2024 08:26
@phts
Copy link
Contributor Author

phts commented Feb 9, 2024

@volumio

integrated github actions with running lint job on every merge request

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