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

Internal: Add prettier and simplify eslint config #530

Merged
merged 13 commits into from Dec 14, 2023

Conversation

beckpaul
Copy link
Collaborator

@beckpaul beckpaul commented Dec 12, 2023

Due to the size of the PR ill be addressing the relevant file changes/new files in comments below.

Main motivation behind this is to address the egregious number of white space issues and large trailing single lines.

Copy link

codecov bot commented Dec 12, 2023

Codecov Report

Attention: 391 lines in your changes are missing coverage. Please review.

Comparison is base (0d21d36) 41.58% compared to head (5cc54dc) 41.17%.

Files Patch % Lines
src/backend/cron-jobs/leaderboardCacheCrawler.js 0.00% 24 Missing ⚠️
src/backend/services/WordpressRepository.js 4.00% 24 Missing ⚠️
...c/backend/routes/views/account/get/connectSteam.js 0.00% 23 Missing ⚠️
src/backend/routes/views/account/post/linkGog.js 0.00% 20 Missing ⚠️
src/backend/routes/views/account/post/register.js 0.00% 20 Missing ⚠️
src/backend/services/ClanManagementService.js 0.00% 20 Missing ⚠️
src/backend/routes/views/clans/create.js 5.26% 18 Missing ⚠️
src/backend/services/ClanManagementRepository.js 0.00% 18 Missing ⚠️
src/backend/services/DataRepository.js 21.73% 18 Missing ⚠️
src/backend/cron-jobs/wordpressCacheCrawler.js 0.00% 13 Missing ⚠️
... and 37 more
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #530      +/-   ##
===========================================
- Coverage    41.58%   41.17%   -0.41%     
===========================================
  Files           71       71              
  Lines         1705     1734      +29     
  Branches       220      210      -10     
===========================================
+ Hits           709      714       +5     
- Misses         964      988      +24     
  Partials        32       32              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -1,7 +1,34 @@
{
Copy link
Collaborator Author

@beckpaul beckpaul Dec 12, 2023

Choose a reason for hiding this comment

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

Add prettier as a plugin for eslint - it gets run now when yarn lint is ran and --fix will also fix prettier issues

Not sure why there were 4 different .eslintrc files in different places but i've removed them and compressed the rules into one.

Copy link
Collaborator

Choose a reason for hiding this comment

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

it's the same with .gitignore, there are multiple ways to configure things, globally or per directory.
i think we can work with overrides for now

@@ -0,0 +1,10 @@
# See https://prettier.io/docs/en/options. most are set to default
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Config for prettier - per comment: most are set to default. We can change this to JSON TOML or plain js if anyone has an aversion to yaml

@@ -34,9 +34,11 @@
"css-loader": "^6.8.1",
"dart-sass": "^1.25.0",
"eslint": "^8.54.0",
"eslint-config-prettier": "^9.1.0",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Add prettier, the plugin to socket it in, and the config that prevents conflicts with eslint.

Essentially we keep eslint as a way to find and prevent bugs, while prettier is soley focused on code styling per:
https://prettier.io/docs/en/comparison

@beckpaul beckpaul self-assigned this Dec 12, 2023
@fcaps
Copy link
Collaborator

fcaps commented Dec 12, 2023

Thx for the work!

not a fan of prettier per se, but i can work with it.
most white-space issues are in other files than .js, that said, how do we fix them now?

@beckpaul
Copy link
Collaborator Author

beckpaul commented Dec 12, 2023

Thx for the work!

not a fan of prettier per se, but i can work with it. most white-space issues are in other files than .js, that said, how do we fix them now?

Something like this: eslint-plugin-pug
will lint the inline scripts for the .pug files, not the "view" itself.

Benefit of prettier here is that we can add: @prettier-plugin-pug
and that will lint the files as a whole and include styling. We can also add some pug specific rules if deemed necessary. List of possibilities: here

Went ahead and added just the prettier plugin in the next few commits and ran it with fix.
Take a look at the changes made to the scripts in package.json to make sure those are fine or if we want to break them out.

Ideally, yarn eslint ... would entail also running prettier on our pug files but haven't gotten a proper config to make that happen so running it explicitly for the time being - will revisit this later today and see if i can clean it up.

package.json Show resolved Hide resolved
@fcaps
Copy link
Collaborator

fcaps commented Dec 13, 2023

@beckpaul you think this can be merged?

@beckpaul
Copy link
Collaborator Author

@beckpaul you think this can be merged?

@fcaps Should be good to go - only concern is if there's any other file types we would want to lint but that could come later since the only type that comes to mind is our CSS which is due for a refactor per another issue.

@fcaps fcaps merged commit 7f8d01b into FAForever:develop Dec 14, 2023
3 of 5 checks passed
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.

None yet

2 participants