Skip to content

Apply eslint and prettier autofixes on codebase #133

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.

Sign up for GitHub

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

Jump to bottom
Merged
tbouffard merged 7 commits into from
Nov 20, 2022
Merged

Apply eslint and prettier autofixes on codebase #133

tbouffard merged 7 commits into from
Nov 20, 2022

Conversation

@mayorovad
Copy link

@mayorovad mayorovad commented Oct 29, 2022

Summary

Theme of this PR is #122. I will try to not create huge PRs on this issue in future, but this one is really big, because auto-scripts applied. I didn't change any code manually.

  • Add missing eslint deps in package.json
  • Run lint:fix and prettier scripts

Description for the changelog

  • Refactor code based on eslint and prettier rules

Other info
Not so sure about eslint rule that eliminates type declarations for variables with defined values 🤔. If it's not ok for you, I can revert it back.

@Kurt29
Copy link

Kurt29 commented Oct 29, 2022

I like that you don't have to add type declarations for inferred default values. And as long as there is no huge other PR right now, it shouldn't be an issue that this branch is a little bigger than usual

@tbouffard
Copy link
Member

tbouffard commented Nov 7, 2022

Sorry for the late answer
I agree with @Kurt29 there are no large PRs, and existing ones have been created by @mayorovad 😸 so we will eventually fix the lint errors in the opened PR afterwards.
I should be able to review this PR soon

tbouffard
tbouffard requested changes Nov 7, 2022
View reviewed changes
Copy link
Member

@tbouffard tbouffard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ I see a lot of remaining errors (confirm when running with GitHub Actions). I suggest we fix them now otherwise new ones will be included in the future.
These errors are not not auto-fixed by eslint so I guess that's why they remain

@mayorovad
chore: eslint - switch no-this-alias error to warn
fa67c24 
Switch rule to warn for now - too many
complicated code that uses `self` alias for this
@mayorovad
refactor: fix eslint errors
5a29d2f 
Eliminate unnecessary code
Eliminate var usage
@mayorovad
refactor: abstract classes, empty methods
a3102af 
Add abstract definition to abstract classes
Fill empty methods blocks with return
Remove unnecessary escapes in regexes
@mayorovad mayorovad mentioned this pull request Nov 20, 2022
Closed
@mayorovad mayorovad requested a review from tbouffard November 20, 2022 12:09
tbouffard
tbouffard approved these changes Nov 20, 2022
View reviewed changes
Copy link
Member

@tbouffard tbouffard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @mayorovad for having fixed all eslint errors 👍🏿

@tbouffard tbouffard merged commit 9e7a603 into maxGraph:development Nov 20, 2022
@tbouffard tbouffard mentioned this pull request Nov 20, 2022
Closed
@mayorovad mayorovad deleted the 122-eslint-fixes branch November 20, 2022 17:23
@tbouffard tbouffard added the chore Build, CI/CD or repository tasks (issues/PR maintenance, environments, ...) label Nov 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tbouffard tbouffard tbouffard approved these changes

Assignees

No one assigned

Labels

chore Build, CI/CD or repository tasks (issues/PR maintenance, environments, ...)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@mayorovad @Kurt29 @tbouffard