Skip to content
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

chore: Remove User.in_test_group?/2, which isn't called anywhere #2357

Merged
merged 1 commit into from
Jan 10, 2024

Conversation

joshlarson
Copy link
Contributor

No description provided.

@joshlarson joshlarson requested a review from a team as a code owner January 10, 2024 16:02
Base automatically changed from dependabot/hex/credo-1.7.2 to main January 10, 2024 16:05
@joshlarson joshlarson force-pushed the jdl/chore/remove-in-test-group branch from de24b7f to 19a2a72 Compare January 10, 2024 16:07
Copy link

Coverage of commit de24b7f

Summary coverage rate:
  lines......: 94.5% (3015 of 3190 lines)
  functions..: 72.9% (1238 of 1699 functions)
  branches...: no data found

Files changed coverage rate:
                                                                  |Lines       |Functions  |Branches    
  Filename                                                        |Rate     Num|Rate    Num|Rate     Num
  ======================================================================================================
  lib/skate/settings/user.ex                                      | 100%     18| 100%     8|    -      0

Download coverage report

Copy link

Coverage of commit 19a2a72

Summary coverage rate:
  lines......: 94.7% (3022 of 3190 lines)
  functions..: 72.9% (1238 of 1699 functions)
  branches...: no data found

Files changed coverage rate:
                                                                  |Lines       |Functions  |Branches    
  Filename                                                        |Rate     Num|Rate    Num|Rate     Num
  ======================================================================================================
  lib/skate/settings/user.ex                                      | 100%     18| 100%     8|    -      0

Download coverage report

@lemald
Copy link
Member

lemald commented Jan 10, 2024

I have slightly mixed feelings on this because we're not using it at the moment but we'll probably use it again plenty of times, but thanks to git history and everything it will be very easy to resurrect this function whenever we need it.

@joshlarson
Copy link
Contributor Author

@lemald I actually wonder if we might be able to use something like

user
|> User.all_test_group_names()
|> Enum.member?("whatever")

instead?

Which would then have an a priori guarantee that listing a user's test groups and checking whether a user is in a test group will never get out of sync with one another (I almost goofed on that when I was adding the override feature.

@joshlarson
Copy link
Contributor Author

But if we think we're going to want to resurrect this, then I almost would rather not delete it.

@lemald
Copy link
Member

lemald commented Jan 10, 2024

@lemald I actually wonder if we might be able to use something like

user
|> User.all_test_group_names()
|> Enum.member?("whatever")

instead?

Which would then have an a priori guarantee that listing a user's test groups and checking whether a user is in a test group will never get out of sync with one another (I almost goofed on that when I was adding the override feature.

Yes, that's a good point, and it's simple enough logic for a user of the test group API to implement. I say go ahead and delete!

@joshlarson joshlarson merged commit d49e802 into main Jan 10, 2024
9 checks passed
@joshlarson joshlarson deleted the jdl/chore/remove-in-test-group branch January 10, 2024 21:32
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.

2 participants