Skip to content

Remove some indirection in IdentityLinker#7420

Merged
zachmargolis merged 5 commits intomainfrom
margolis-identity-linker-cleanup
Jan 3, 2023
Merged

Remove some indirection in IdentityLinker#7420
zachmargolis merged 5 commits intomainfrom
margolis-identity-linker-cleanup

Conversation

@zachmargolis
Copy link
Contributor

@zachmargolis zachmargolis commented Dec 2, 2022

Small proposed change related to #7418 (or possibly the next PR after, where we'll to conditionally create ServiceProviderIdentity rows but with fewer attributes)

Originally started as a patch to #7418, but that may not land, and I think these changes would be good regardless

Comment on lines 85 to 80
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had two motivations here:

  1. rename to clarify that this is about verified_attributes (since there were like five _attributes methods and that wasn't clear
  2. I don't like the pattern of re-assigning the verified_attributes argument and figured it was easier to inline and splat

Copy link
Contributor

@n1zyy n1zyy left a comment

Choose a reason for hiding this comment

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

I like this change. The existing code was a little bit hard to follow and this cleans it up nicely.

I think this is potentially more logically related to the (not yet written) next PR in the series, but I'm game to merge it here since it cleans things up.

Copy link
Contributor

Choose a reason for hiding this comment

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

Was this just a typo in the example name? o_O

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes as far as I could tell

@zachmargolis
Copy link
Contributor Author

I think this is potentially more logically related to the (not yet written) next PR in the series, but I'm game to merge it here since it cleans things up.

We can merge this as a fast-follow to your PR, so to keep each diff small

- Remove more intermediate methods
- Inline a bunch
- Spec cleanup
@zachmargolis zachmargolis force-pushed the margolis-identity-linker-cleanup branch from bb179dd to 2a0064d Compare December 30, 2022 17:39
@zachmargolis zachmargolis changed the base branch from mattw/LG-8058_default_scope to main December 30, 2022 17:40
@zachmargolis
Copy link
Contributor Author

I'm updating this branch to go directly to main, I think that these refactors might be nice for readability separate from that other code

@zachmargolis zachmargolis requested a review from a team December 30, 2022 17:42
@zachmargolis zachmargolis merged commit 4fb2f3e into main Jan 3, 2023
@zachmargolis zachmargolis deleted the margolis-identity-linker-cleanup branch January 3, 2023 19:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants