Skip to content

Rename executePoll -> _executePoll + stopPollingByNetworkClientId -> stopPollingByPollingToken - #1810

Merged
adonesky1 merged 5 commits into
mainfrom
pollingcontroller-cleanups
Oct 12, 2023
Merged

Rename executePoll -> _executePoll + stopPollingByNetworkClientId -> stopPollingByPollingToken#1810
adonesky1 merged 5 commits into
mainfrom
pollingcontroller-cleanups

Conversation

@adonesky1

@adonesky1adonesky1 commented Oct 11, 2023•
edited
Loading

Copy link
Copy Markdown
Contributor

Fast-follow to #1776, just a few (breaking) naming cleanups

CHANGED

  • BREAKING:executePoll renamed to _executePoll
  • BREAKING:stopPollingByNetworkClientId renamed to stopPollingByPollingToken

@adonesky1
adonesky1 requested a review from a team as a code ownerOctober 11, 2023 21:34
@adonesky1
adonesky1force-pushed the pollingcontroller-cleanups branch from 9a4214b to 41c4258CompareOctober 11, 2023 21:52
shanejonas
shanejonas previously approved these changes Oct 12, 2023
View reviewed changes
mcmire
mcmire reviewed Oct 12, 2023
View reviewed changes

@mcmiremcmire left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The new names make sense! I just had some suggestions on further communicating that _executePoll is private, and for being consistent internally in treating this method as though it were truly private.

Comment threadpackages/assets-controllers/src/TokenListController.test.ts Outdated

@mcmiremcmireOct 12, 2023•
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Do we want to keep these tests if we are essentially making this method private? We don't conventionally test private code; we should be able to rename this method now without the tests breaking. Is there some other place we could move these tests to?

@adonesky1adonesky1Oct 12, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

removed this block and moved the test into the startPollingByNetworkClientId test block: 7a40412

Comment threadpackages/gas-fee-controller/src/GasFeeController.test.ts Outdated

@mcmiremcmireOct 12, 2023

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Similar for these tests.

@adonesky1adonesky1Oct 12, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

removed this block and moved the test into the startPollingByNetworkClientId test block: 7a40412

@mcmiremcmireOct 12, 2023

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Hmm, I see the changes for TokenListController, those look great — do we want to do the same for this controller?

@adonesky1adonesky1Oct 12, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

sorry whoops so in that commit I linked I moved some of the logic into the existing startPollingByNetworkClientId test but forgot to remove this block. That removal is done here: b9502fc

Comment threadpackages/polling-controller/src/PollingController.test.ts Outdated
Comment threadpackages/assets-controllers/src/TokenListController.ts Outdated
Comment threadpackages/gas-fee-controller/src/GasFeeController.ts Outdated
Comment threadpackages/polling-controller/src/PollingController.ts Outdated
@adonesky1
adonesky1force-pushed the pollingcontroller-cleanups branch from 9f870a6 to 7a40412CompareOctober 12, 2023 22:52
mcmire
mcmire approved these changes Oct 12, 2023
View reviewed changes

@mcmiremcmire left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM!

@adonesky1
adonesky1 merged commit 0fd4b39 into mainOct 12, 2023
@adonesky1
adonesky1 deleted the pollingcontroller-cleanups branch October 12, 2023 23:52
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcmiremcmiremcmire approved these changes
@jiexijiexiAwaiting requested review from jiexi
@BelfordZBelfordZAwaiting requested review from BelfordZ
@shanejonasshanejonasAwaiting requested review from shanejonas

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@adonesky1@mcmire@shanejonas