Skip to content

Changing actor name updates mappings and credentials store #285

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.

Sign up for GitHub

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
Merged
merged 22 commits into from
Nov 19, 2020
Merged

Changing actor name updates mappings and credentials store #285

merged 22 commits into from
Nov 19, 2020

Conversation

silverbucket
Copy link
Member

  • updateCredentials -> updateActor
  • Mappings and store are updated with new lookup identifier (hash of actor string)
  • Destruction of platformInstance consolidated and simplified
  • Additional unit tests and coverage

Resolves #269

@silverbucket silverbucket added this to the Core milestone Nov 17, 2020
@silverbucket silverbucket requested a review from galfert November 17, 2020 12:51
@silverbucket silverbucket self-assigned this Nov 17, 2020
@lgtm-com
Copy link

lgtm-com bot commented Nov 17, 2020

This pull request fixes 9 alerts when merging f054307 into c9271c3 - view on LGTM.com

fixed alerts:

  • 5 for Unused variable, import, function or class
  • 3 for Useless conditional
  • 1 for Identical operands

@silverbucket silverbucket requested a review from raucao November 17, 2020 13:20
@raucao
Copy link
Contributor

raucao commented Nov 17, 2020 •
edited
Loading

Nice!

The Travis build is still failing. I'll review when it's green, so I can just go by the tests.

@silverbucket
Copy link
Member Author

@raucao seems to be failing to get a jest command which was listed as a peer dependency, just moved it to a dev dependency so hopefully that fixes it.

@lgtm-com
Copy link

lgtm-com bot commented Nov 17, 2020

This pull request fixes 9 alerts when merging 2fa86db into c9271c3 - view on LGTM.com

fixed alerts:

  • 5 for Unused variable, import, function or class
  • 3 for Useless conditional
  • 1 for Identical operands

@silverbucket
Copy link
Member Author

@raucao seems like things are passing and green now

@lgtm-com
Copy link

lgtm-com bot commented Nov 17, 2020

This pull request fixes 9 alerts when merging a56bf94 into c9271c3 - view on LGTM.com

fixed alerts:

  • 5 for Unused variable, import, function or class
  • 3 for Useless conditional
  • 1 for Identical operands

@lgtm-com
Copy link

lgtm-com bot commented Nov 17, 2020

This pull request fixes 9 alerts when merging 4f3db4a into c9271c3 - view on LGTM.com

fixed alerts:

  • 5 for Unused variable, import, function or class
  • 3 for Useless conditional
  • 1 for Identical operands

@silverbucket silverbucket changed the title Changing actor name updates lookup mappings and credentials store Changing actor name updates mappings and credentials store Nov 18, 2020
@silverbucket
Copy link
Member Author

silverbucket commented Nov 18, 2020 •
edited
Loading

@galfert or @raucao this PR is ready for review.

raucao
raucao reviewed Nov 19, 2020
View reviewed changes
Copy link
Contributor

@raucao raucao left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Left some comments. Love that there are more tests now!

One thing I would consider when using spec syntax like "describe", is to also use spec language instead of "should". For example "it should generate a failure report" would turn into "it generates a failure report", which both removes the superfluous extra word, as well as reads more like a specification than a wish list. Just my personal opinion though.

Btw, I think this PR is more of a kredits-2. The 3 is really a bit of a nuclear option for when it's most definitely more than 3 mediums, and usually even more than that. I had plenty of mediums that I spent more than a day working on for example. The incentive should always be to make PRs smaller (and thus easier to review and reason about) instead of slapping a 3 on ones that contain too many side quests. :)

@silverbucket
Copy link
Member Author

Left some comments. Love that there are more tests now!

One thing I would consider when using spec syntax like "describe", is to also use spec language instead of "should". For example "it should generate a failure report" would turn into "it generates a failure report", which both removes the superfluous extra word, as well as reads more like a specification than a wish list. Just my personal opinion though.

Yeah, good suggestion. I'll do that.

Btw, I think this PR is more of a kredits-2. The 3 is really a bit of a nuclear option for when it's most definitely more than 3 mediums, and usually even more than that. I had plenty of mediums that I spent more than a day working on for example. The incentive should always be to make PRs smaller (and thus easier to review and reason about) instead of slapping a 3 on ones that contain too many side quests. :)

OK, I am still pretty unsure of when to use which, so I'll try to remember that rule of thumb.

@silverbucket silverbucket requested a review from raucao November 19, 2020 15:17
@silverbucket silverbucket added kredits-2 Medium contribution and removed kredits-3 Large contribution labels Nov 19, 2020
raucao
raucao reviewed Nov 19, 2020
View reviewed changes
Copy link
Contributor

@raucao raucao left a comment

Choose a reason for hiding this comment

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

Just some typos...

silverbucket and others added 3 commits November 19, 2020 17:57
@silverbucket
Update packages/sockethub/src/platform-instance.test.ts
0bbe815 
Co-authored-by: Râu Cao <sebastian@kip.pe>
@silverbucket
Update packages/sockethub/src/platform-instance.test.ts
3f34944 
Co-authored-by: Râu Cao <sebastian@kip.pe>
@silverbucket
Update packages/sockethub/src/platform-instance.test.ts
d5ae4e2 
Co-authored-by: Râu Cao <sebastian@kip.pe>
@silverbucket silverbucket merged commit 59d2a49 into master Nov 19, 2020
@silverbucket silverbucket deleted the feature/store_updated_credentials branch November 19, 2020 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@raucao raucao raucao left review comments

@galfert galfert Awaiting requested review from galfert

Assignees

@silverbucket silverbucket

Labels
kredits-2 Medium contribution state:ready type:feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Re-implement updateCredentials
2 participants
@silverbucket @raucao