Skip to content

/ ghostery-extension

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

Jump to bottom

Code Cleanup: Enforce Linting Rules & Update UNSAFE_ React Lifecycle Events #559

Merged
merged 33 commits into from Jun 4, 2020
Merged

Code Cleanup: Enforce Linting Rules & Update UNSAFE_ React Lifecycle Events #559

merged 33 commits into from Jun 4, 2020

Conversation

@IAmThePan
Copy link
Contributor

@IAmThePan IAmThePan commented May 26, 2020

  • Have you followed the guidelines in CONTRIBUTING.md?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Have you added an explanation of what your changes do?
  • Does your submission pass tests?
  • Did you lint your code prior to submission?

From Jira Cleanup Ticket (that I don't have access to):

  • Clean up eslintrc.js:
    • In eslintrc.js, enable any checks marked with TODOs and fix associated code warnings.
    • Additionally remove template-curly-spacing and enable class-methods-use-this, no-mixed-operators, import/prefer-default-export.
  • In React components, search for any life-cycle events marked with UNSAFE_ and refactor to use new React events.
IAmThePan added 29 commits Apr 29, 2020
@IAmThePan
add linting for no-param-reassign and fix resulting linting errors
75b0091
@IAmThePan
add linting for prefer-object-spread and fix resulting linting errors
8d37082
@IAmThePan
add linting for no-restricted-syntax and fix 1/2 of resulting errors
3049bd1
@IAmThePan
add linting for no-prototype-builtins and fix resulting linting errors
fe03ad7
@IAmThePan
add linting for class-methods-use-this and fix most resulting errors.…
f11bc18 
… Add /tools and /tests to linter. Update height in UpgradeBanner
@IAmThePan
finish linting for class-methods-use-this
5f3471e
@IAmThePan
add linting for no-mixed-operators and fix resulting linting errors
d4d84dc
@IAmThePan
add linting for import/prefer-default-export and fix resulting lintin…
17f90ed 
…g errors
@IAmThePan
add linting for react/no-access-state-in-setstate and fix resulting l…
a77cd1b 
…inting errors
@IAmThePan
add linting for react/jsx-props-no-spreading and fix resulting lintin…
c31dbaf 
…g errors
@IAmThePan
finish linting errors for no-restricted-syntax. 1 remains: couldn't r…
3d50aff 
…esolve removing iterator loops for urlSearchParams
@IAmThePan
Merge branch 'develop' into feature/update-linter
227d012
@IAmThePan
Fix linting errors resulting from the merge with develop
Loading status checks…
0988a29
@IAmThePan
Refactor UNSAFE_componentWillMount into either constructor or compone…
b55be21 
…ntDidMount, leave notes for how decision was made.
@IAmThePan
Refactor UNSAFE_componentWillReceiveProps to componentDidUpdate or ge…
Loading status checks…
241747c 
…tDerivedStateFromProps
@IAmThePan
re-enable lint exception for no-prototype-builtins and revert calls b…
eb413fb 
…ack to hasOwnProperty
@IAmThePan
add single line exception for no-restricted-syntax linting rule
ac0ec93
@IAmThePan
add linting for react/destructuring-assignment and fix errors. ToDo: …
fd954eb 
…test code and check for errors
@IAmThePan
Fix minor bugs
e815546
@IAmThePan
Fix General Settings last updated text
ae8ffcc
@IAmThePan
rework linting rule no-param-reassign to have more exceptions and par…
4c56460 
…am object destructuring
@IAmThePan
Remove file and line linting exceptions.
c5ef663
@IAmThePan
re-add linting rule react/sort-comp and fix resulting errors
3ca2402
@IAmThePan
remove added linting exception consistent-return and fix resulting er…
4533fd7 
…rors
@IAmThePan
remove added linting expression no-use-before-define and fix resultin…
590aa60 
…g errors. fix BugDB.js bug.
@IAmThePan
Fix linting error
ecb62e8
@IAmThePan
fix minor bugs
f634c1d
@IAmThePan
Code cleanup: fix PromoModal imports
ea51432
@IAmThePan
Merge with develop. Fix resulting linting errors
Loading status checks…
75a88cb
@IAmThePan IAmThePan requested a review from christophertino as a code owner May 26, 2020
@IAmThePan IAmThePan requested review from Eden12345, jsignanini, wlycdgr, zarembsky and ghostery/ghostery as code owners May 26, 2020
@IAmThePan IAmThePan mentioned this pull request May 26, 2020
Closed
5 of 5 tasks complete
IAmThePan added 4 commits May 28, 2020
@IAmThePan
remove unnecessary hasOwnProperty calls after refactored for...in loops
Loading status checks…
68493d2
@IAmThePan
Fix missing strings bug
6297062
@IAmThePan
Fix last remaining string bug
9b4d16c
@IAmThePan
Merge branch 'develop' into feature/cleanup
Loading status checks…
2cb47a1
@christophertino christophertino merged commit 3de53c2 into ghostery:develop Jun 4, 2020
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@christophertino christophertino added this to the 8.5.2 milestone Jun 4, 2020
@christophertino
christophertino reviewed Jun 4, 2020
View changes
Copy link
Member

@christophertino christophertino left a comment

Reviewed first 70 files. @IAmThePan just a few things we can remove from .eslintrc.js
app/hub/Views/CreateAccountView/CreateAccountView.jsx Show resolved Hide resolved
.eslintrc.js Show resolved Hide resolved
.eslintrc.js Show resolved Hide resolved
.eslintrc.js Show resolved Hide resolved
.eslintrc.js Show resolved Hide resolved
.eslintrc.js Show resolved Hide resolved
.eslintrc.js Show resolved Hide resolved
.eslintrc.js Show resolved Hide resolved
@wlycdgr
wlycdgr reviewed Jun 4, 2020
View changes
Copy link
Member

@wlycdgr wlycdgr left a comment

Left a few comments/questions on files 70-140
app/panel-android/components/Panel.jsx Show resolved Hide resolved
app/panel-android/components/content/Accordion.jsx Show resolved Hide resolved
app/panel-android/components/content/Accordions.jsx Show resolved Hide resolved
app/panel-android/components/content/ChartSVG.jsx Show resolved Hide resolved
app/panel-android/components/content/DotsMenu.jsx Show resolved Hide resolved
app/panel/components/Blocking/Tracker.jsx Show resolved Hide resolved
app/panel/components/BuildingBlocks/DonutGraph.jsx Show resolved Hide resolved
app/panel/components/Settings/GeneralSettings.jsx Show resolved Hide resolved
app/panel/components/Settings/TrustAndRestrict.jsx Show resolved Hide resolved
app/panel/components/Summary.jsx Show resolved Hide resolved
@IAmThePan IAmThePan deleted the IAmThePan:feature/cleanup branch Jun 6, 2020
@IAmThePan IAmThePan restored the IAmThePan:feature/cleanup branch Jun 11, 2020
@IAmThePan
Copy link
Contributor Author

@IAmThePan IAmThePan commented Jun 11, 2020

I restored the IAmThePan:feature/cleanup branch hoping that it would track the updates I made to address the comments. It didn't so you can see the changes on IAmThePan:feature/cleanup-cleanup which also has a PR: #566
@wlycdgr
wlycdgr reviewed Jun 11, 2020
View changes
app/panel/reducers/panel.js Show resolved Hide resolved
app/panel/reducers/panel.js Show resolved Hide resolved
app/panel/reducers/settings.js Show resolved Hide resolved
app/panel/reducers/settings.js Show resolved Hide resolved
app/scss/partials/_upgrade_banner.scss Show resolved Hide resolved
src/background.js Show resolved Hide resolved
src/classes/Account.js Show resolved Hide resolved
src/classes/Account.js Show resolved Hide resolved
src/classes/Account.js Show resolved Hide resolved
src/classes/BugDb.js Show resolved Hide resolved
@wlycdgr
wlycdgr reviewed Jun 11, 2020
View changes
src/classes/FoundBugs.js Show resolved Hide resolved
src/classes/FoundBugs.js Show resolved Hide resolved
src/classes/Module.js Show resolved Hide resolved
src/classes/PromoModals.js Show resolved Hide resolved
src/classes/SurrogateDb.js Show resolved Hide resolved
src/utils/utils.js Show resolved Hide resolved
@IAmThePan
Copy link
Contributor Author

@IAmThePan IAmThePan commented Jun 12, 2020

I addressed all outstanding comments and pushed my code changes to the feature/cleanup-cleanup branch and PR.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@christophertino christophertino

@wlycdgr wlycdgr

@Eden12345 Eden12345

@jsignanini jsignanini

@zarembsky zarembsky

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
8.5.2
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
@IAmThePan @christophertino @wlycdgr