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

Enable ESLint on all js files #1270

Closed
2 tasks
nichhk opened this issue Jul 7, 2022 · 7 comments · Fixed by #1331
Closed
2 tasks

Enable ESLint on all js files #1270

nichhk opened this issue Jul 7, 2022 · 7 comments · Fixed by #1331
Assignees
Labels
Complexity: Missing This ticket needs a complexity (good first issue, small, medium, or large) Feature: Code Health Make our code more readable, testable, and modular Milestone: Missing Role: Frontend React front end work size: 8pt Can be done in 31-48 hours Tech Stack: New

Comments

@nichhk
Copy link
Member

nichhk commented Jul 7, 2022

Overview

Many of our javascript files say /* eslint-disable */ at the top, preventing the linter from running on them. This is a bad idea since the linter helps to identify readability issues and bugs.

Find the full list of places where we're disabling ESLint here: https://github.com/hackforla/311-data/search?q=%22%2F*+eslint-disable+*%2F%22. Note that we do NOT care about code in client/v1!

After we enable ESLint on all the files, we should enable simple-import-sort to sort our imports.

Action Items

  • Remove /* eslint-disable */ from all files and fix the resulting errors. We should break this up into multiple PRs--one per file, or one per directory.
  • Enable simple-import-sort and fix resulting errors
@nichhk nichhk self-assigned this Jul 7, 2022
@nichhk nichhk added Role: Frontend React front end work Feature: Code Health Make our code more readable, testable, and modular size: 8pt Can be done in 31-48 hours labels Jul 19, 2022
@nichhk nichhk added this to the v2.1 Launch milestone Jul 19, 2022
@nichhk
Copy link
Member Author

nichhk commented Aug 2, 2022

@funbunch and @ardada2468, feel free to work on this issue if you have extra time. Please note the files that you'll be fixing before starting on them here, so that we can avoid duplicate work!

@edwinjue
Copy link
Member

ran the following to find places where /* eslint-disable */ is located (excluding v1, node_modules directories and the compiled code bunde.js):

grep -Rin --exclude-dir={v1,node_modules} --exclude=bundle.js eslint-disable

here are the locations and line numbers. hope this helps

client/components/main/Reports.jsx:1:/* eslint-disable react/self-closing-comp */

client/components/main/Reports.jsx:31: /* eslint-disable consistent-return */

client/components/main/CookieNotice.jsx:1:/* eslint-disable */

client/components/main/Desktop/TypeSelector/index.js:1:/* eslint-disable */

client/components/common/MultiSelect/GroupedMultiSelect.jsx:39: // eslint-disable-next-line

client/components/Map/Map.jsx:1:/* eslint-disable */

client/components/Map/mapColors.js:1:/* eslint-disable */

client/components/Map/constants.js:1:/* eslint-disable */

client/components/Map/districts.js:1:/* eslint-disable */

client/components/Map/index.js:1:/* eslint-disable */

client/components/Map/layers/BoundaryLayer.js:1:/* eslint-disable */

client/components/Map/layers/RequestsLayer.js:1:/* eslint-disable */

client/components/Map/layers/AddressLayer.js:1:/* eslint-disable */

client/components/Map/controls/MapOverview.jsx:1:/* eslint-disable */

client/components/Map/controls/RequestsBarChart.jsx:1:/* eslint-disable */

client/components/Map/controls/MapRegion.jsx:1:/* eslint-disable */

client/components/Map/controls/MapSearch.jsx:1:/* eslint-disable */

client/components/Map/controls/MapLayers.jsx:1:/* eslint-disable */

client/components/Map/controls/MapMeta.jsx:1:/* eslint-disable */

client/components/Map/controls/RequestsDonut.jsx:1:/* eslint-disable */

client/components/Map/RequestDetail.jsx:1:/* eslint-disable react/prop-types */

client/components/Map/geoUtils.js:1:/* eslint-disable */

client/App.jsx:42: {/* eslint-disable-next-line react/jsx-props-no-spreading */}

client/index.js:1:/* eslint-disable react/jsx-filename-extension */

client/redux/tempTypes.js:1:/* eslint-disable */

client/redux/sagas/data.js:1:/* eslint-disable */

client/utils/checkEnv.js:1:/* eslint-disable no-console */

@nichhk
Copy link
Member Author

nichhk commented Aug 21, 2022

Thanks for this list Edwin! I will claim everything in "client/components/Map/controls/".

@nichhk nichhk assigned edwinjue and jekijo and unassigned ardada2468 Aug 22, 2022
@edwinjue
Copy link
Member

Glad I could help! However, I notice branch 1270-enable-eslint-on-all-js-files is 100 commits behind dev. Should we still checkout this branch to make the changes or should we go off a new branch?

@edwinjue
Copy link
Member

Since I'm already working on the contact form, I'll lint everything in client/components/main/

@edwinjue
Copy link
Member

edwinjue commented Aug 27, 2022

Linted everything except the following:

client/components/Map/*
client/redux/tempTypes.js:1:/* eslint-disable */
client/components/common/MultiSelect/GroupedMultiSelect.jsx:39: // eslint-disable-next-line
client/utils/checkEnv.js:1:/* eslint-disable no-console */

Changes have been pushed up to this branch. Will try to take a look at some of the other Map files when I find more free time

@edwinjue edwinjue linked a pull request Aug 29, 2022 that will close this issue
4 tasks
@edwinjue edwinjue reopened this Aug 31, 2022
@EchoProject EchoProject removed this from the v2.1 Launch milestone Dec 8, 2022
@mc759
Copy link
Member

mc759 commented Dec 13, 2022

Hey @edwinjue Do you have an update for us on this issue?

Please update:

  • Progress:
  • Blockers:
  • Availability:
  • ETA:

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Complexity: Missing This ticket needs a complexity (good first issue, small, medium, or large) Feature: Code Health Make our code more readable, testable, and modular Milestone: Missing Role: Frontend React front end work size: 8pt Can be done in 31-48 hours Tech Stack: New
Projects
Status: Done (without merge)
Development

Successfully merging a pull request may close this issue.