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 eslint for all files #755

Draft
wants to merge 40 commits into
base: master
Choose a base branch
from

Conversation

modos189
Copy link
Contributor

Fixed the style, in some cases minor refactoring or bug fixes have been made, in such cases I tried to mention it in the commit message

  • Fix eslint for core files
  • Fix eslint for plugins

@modos189 modos189 added the development general development issue label Aug 28, 2024
@xscreach
Copy link
Contributor

Frankly, i was saying we should do this long time ago, but the concensus from TG was that it will mess with history making it harder to find the real "owner" of the changes thus "we" don't want this to happen...

Did this change? I'm personally all for this, but we agreed differently... so... 🤷‍♂️

Are "we" really ok with it? 🤷‍♂️

@modos189
Copy link
Contributor Author

Really, I didn't want to do it before. But it turned out that so much time has passed, and we still have a lot of code that does not conform to the style, which makes it difficult to understand and complicates the preparation of new changes.
So I think we need to bring the files to the style. I want to know what other people think

@xscreach
Copy link
Contributor

Really, I didn't want to do it before. But it turned out that so much time has passed, and we still have a lot of code that does not conform to the style, which makes it difficult to understand and complicates the preparation of new changes.
So I think we need to bring the files to the style. I want to know what other people think

Cool, cool 👍
I'm all for/pro this thing. As you said, it will make new changes easier in terms of preparing "green" PR, lot fewer people pissed from the constant eslint complains and code better readable (at least not all red in IDE cause of broken codestyle 🤪), etc...

I'll just note that we should test this PR before merging as it's changing every single file (or like 99% of them). It should be all good... it's just eslint fix... but... it could also be, for some stupid reason, very bad 😬

@modos189
Copy link
Contributor Author

I'll just note that we should test this PR before merging as it's changing every single file (or like 99% of them). It should be all good... it's just eslint fix... but... it could also be, for some stupid reason, very bad 😬

Yes, it's true. For example, I had to add some eslint-disable-next-line in boot.js. After the fixes, the build completed successfully and I didn't notice any errors, but I might have missed something

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
development general development issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants