Skip to content

implement a skip_config_creation option in get_user_info #250

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

Closed
wants to merge 2 commits into from

Conversation

yucongalicechen
Copy link
Contributor

closes #244
@sbillinge ready for some feedback
btw I see in the docs you used get_user_data instead of get_user_info at a couple places, which seems to be a better name, shall we change to get_user_data instead?

Copy link

codecov bot commented Dec 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (2ccd7d6) to head (73fc35e).
Report is 10 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #250   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            8         8           
  Lines          380       390   +10     
=========================================
+ Hits           380       390   +10     
Files with missing lines Coverage Δ
tests/test_tools.py 100.00% <100.00%> (ø)

@@ -22,6 +22,9 @@ def _run_tests(inputs, expected):
config = get_user_info(args)
assert config.get("username") == expected_username
assert config.get("email") == expected_email
config = get_user_info(args, skip_config_creation=True)
assert config.get("username") == expected_username
assert config.get("email") == expected_email
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 looks like we can reuse this run_test function for most test cases for different skip_config_creation except when there're no inputs or files, so I added this here

# Test skipping config creation, expecting None values
config = get_user_info(args, skip_config_creation=True)
assert config.get("username") is None
assert config.get("email") is None
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have to rewrite this from run_test for no args/inputs/config files

@sbillinge
Copy link
Contributor

closes #244 @sbillinge ready for some feedback btw I see in the docs you used get_user_data instead of get_user_info at a couple places, which seems to be a better name, shall we change to get_user_data instead?

haha, that was a mistake on my part. I am not sure tbh. I am ok with either "user-data* or "user-info". @bobleesj @yucongalicechen which do you think is more obvious?

Copy link
Contributor

@sbillinge sbillinge left a comment

Choose a reason for hiding this comment

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

tbh I find the tests confusing for the get_user_info/data workflow. Can we maybe have them written following the new standards that @bobleesj and I worked out?

assert config.get("email") == expected_email

# Test skipping config creation, expecting None values
config = get_user_info(args, skip_config_creation=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

I am a little confused here....isn't this getting args as written?

@yucongalicechen
Copy link
Contributor Author

closed as replaced by #253

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.

implement a skip_config_creation option in get_user_info
2 participants