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

Default user #80

Merged
merged 8 commits into from
Mar 28, 2023
Merged

Default user #80

merged 8 commits into from
Mar 28, 2023

Conversation

kp-cat
Copy link
Contributor

@kp-cat kp-cat commented Mar 22, 2023

Describe the purpose of your pull request

  • In the client options, you can set a default user object used when there's no user passed to get_value() / get_all_values() / get_variation_id() / get_all_variation_ids() methods.
  • set_default_user(user) / clear_default_user() methods to set / remove a default user object used when there's no user passed to get_value() / get_all_values() / get_variation_id() / get_all_variation_ids() methods.
  • Fix broken test
  • Add elixir 1.14.x version to the auto tests
  • In the client options, the following params changed:
    • connect_timeout to connect_timeout_milliseconds (to indicate the unit of measure).
    • read_timeout to read_timeout_milliseconds (to indicate the unit of measure).

Related issues (only if applicable)

https://trello.com/c/XneffR1g/270-default-user-template-2022-09-05-elixir

Requirement checklist (only if applicable)

  • I have covered the applied changes with automated tests.
  • I have executed the full automated test set against my changes.
  • I have validated my changes against all supported platform versions.
  • I have read and accepted the contribution agreement.

@kp-cat kp-cat requested a review from a team as a code owner March 22, 2023 14:37
@kp-cat kp-cat requested a review from randycoulman March 22, 2023 14:38
@codecov
Copy link

codecov bot commented Mar 22, 2023

Codecov Report

Patch coverage: 69.23% and project coverage change: -0.66 ⚠️

Comparison is base (d7f0d54) 92.65% compared to head (fd61ca2) 92.00%.

❗ Current head fd61ca2 differs from pull request most recent head f956e86. Consider uploading reports for the commit f956e86 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #80      +/-   ##
==========================================
- Coverage   92.65%   92.00%   -0.66%     
==========================================
  Files          19       19              
  Lines         504      513       +9     
==========================================
+ Hits          467      472       +5     
- Misses         37       41       +4     
Impacted Files Coverage Δ
lib/config_cat.ex 68.65% <0.00%> (-4.36%) ⬇️
lib/config_cat/client.ex 95.71% <100.00%> (+0.32%) ⬆️
lib/config_cat/config_fetcher.ex 97.29% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Collaborator

@randycoulman randycoulman left a comment

Choose a reason for hiding this comment

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

These changes seem reasonable. One comment about the tests below.

The name change on the config settings will be a breaking change, so you'll likely want to bump the major version when you release it.

Comment on lines +95 to +98
test "get_value/4 uses the default user if no user is passed", %{client: client} do
Client.set_default_user(client, User.new("test@test1.com"))
assert Client.get_value(client, "testStringKey", "") == "fake1"
end
Copy link
Collaborator

Choose a reason for hiding this comment

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

This test seems like a duplicate of the ones in the describe block above, and doesn't seem to belong in a describe block labelled "when the default user is not defined".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should mean that the default_user is not predefined in the options. Maybe it would be better if the describe block would be this: "when the default user is not defined in the options"

@kp-cat kp-cat merged commit 2a0efb7 into main Mar 28, 2023
@kp-cat kp-cat deleted the default-user branch March 28, 2023 20:23
@kp-cat kp-cat mentioned this pull request Sep 20, 2023
4 tasks
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