Skip to content

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?

@codecov
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%> (ø)

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