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

Implement linting #59

Open
jalezi opened this issue Dec 3, 2021 · 9 comments
Open

Implement linting #59

jalezi opened this issue Dec 3, 2021 · 9 comments
Labels
question Further information is requested

Comments

@jalezi
Copy link
Collaborator

jalezi commented Dec 3, 2021

@stefanb @lukarenko @miaerbus @mihaerzen @overlordtm @bananica
First of all I would like to thank you all for your contribution so far and that you are patient with me :).

Before I or someone else really tackle #24 and #30 I would like to have code as clean as possible in given moment. I am afraid of large number of conflicts. The second reason is very annoying. Switching between linting and other branches requires, at least from my experience, deleting /node_modules and re-installing them and I would really love to have linting enabled.

Unless there is better way, I suggest that we merge all open PRs, even thou they might not completely solve issues or implement new features. Apply linting and then continue.

If we decide so, @mihaerzen, when should we turn on rules that are currently turned off? I assume immediately after previously rule is applied. And after all rules are applied we should continue with fixing issues and implementing new features. Will it take too long to apply all the rules? What would you do?

@jalezi jalezi added the question Further information is requested label Dec 3, 2021
@mihaerzen
Copy link
Collaborator

I would suggest to merge the current lintig PR now. Afterwards we can turn the remaining rules ON one by one and create pull requests for those separately so we don't introduce too many changes in one shot.

@stefanb
Copy link
Member

stefanb commented Dec 3, 2021

As long as the builds won't be failing after #47 is merged, then continue with smaller PRs, enabling additional rules and fixing the code as required.

@mihaerzen
Copy link
Collaborator

By "builds won't be failing" you mean Docker image still build or something else @stefanb ? I have run the docker build and it works just fine. Also, I haven't really made any changes to the current build process but rather introduced a new git action that only runs on PRs being opened/updated.

@stefanb
Copy link
Member

stefanb commented Dec 3, 2021

then i suggest we merge it asap!

@stefanb
Copy link
Member

stefanb commented Dec 3, 2021

merged #47, #60, #61. Everything still works 🎉

@stefanb
Copy link
Member

stefanb commented Dec 5, 2021

Should we enable any other rules?

@jalezi
Copy link
Collaborator Author

jalezi commented Dec 6, 2021

Should we enable any other rules?

I don't think it's good idea. Some changes based on rules might require changes all over the code.

What I did is that I turned them on and try to fix specific file, component or just part of the code.
I did it here.

To run eslint for specific file just run in terminal yarn eslint path/to/file.

Be careful and don't commit .eslintrc with rules turned on.

@mihaerzen
Copy link
Collaborator

I've made a PR for no-underscore-dangle (#80) AND I have turned on the warnings for the remaining two, so at least you get some info when adding new features.

I would be in favour of enabling them and fixing them (I can do it) as they are improving the overall code quality and stability.

@jalezi
Copy link
Collaborator Author

jalezi commented Dec 6, 2021

@mihaerzen You beat me on no-underscore-dangle I was just heading to solve it when I noticed your PR.

One more thing. I have never written tests for react. Can you do one or two as examples?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants