hackforla / 311-data

Empowering Neighborhood Associations to improve the analysis of their initiatives using 311 data
https://hackforla.github.io/311-data/
GNU General Public License v3.0
62 stars 64 forks source link

Enable ESLint on all js files #1270

Closed nichhk closed 1 year ago

nichhk commented 2 years ago

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

nichhk commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

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

edwinjue commented 2 years ago

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 commented 2 years ago

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

edwinjue commented 2 years ago

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

mc759 commented 1 year ago

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

Please update:

Thanks!