change args to user_info and fix docstring #253

yucongalicechen · 2024-12-19T02:18:09Z

closes #245, closes #244
@sbillinge ready for review

codecov · 2024-12-19T02:19:43Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (641a107) to head (4cfd13e).
Report is 58 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##              main      #253   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            8         8           
  Lines          380       379    -1     
=========================================
- Hits           380       379    -1

Files with missing lines	Coverage Δ
tests/test_tools.py	`100.00% <100.00%> (ø)`

src/diffpy/utils/tools.py

sbillinge · 2024-12-19T13:36:04Z

I am still struggling a bit with this functionality. I think maybe I got to the botto of why this is the case (though maybe not!). I think the function is trying to do two things which is making me confused. The two things are (a) getting the config information from config files and (b) a workflow to create/maintain the config file. I wonder if this all gets a bit easier if we separate these behaviorws, for example, with a conditional, but this taking place outside the get_info function? Maybe we could sketch out this flow in a diagram?

yucongalicechen · 2024-12-19T19:55:56Z

I am still struggling a bit with this functionality. I think maybe I got to the botto of why this is the case (though maybe not!). I think the function is trying to do two things which is making me confused. The two things are (a) getting the config information from config files and (b) a workflow to create/maintain the config file. I wonder if this all gets a bit easier if we separate these behaviorws, for example, with a conditional, but this taking place outside the get_info function? Maybe we could sketch out this flow in a diagram?

Yes, I agree we can make it a bit more readable! I drew a diagram on this. I am thinking that we can have one private function for each action in the diagram?

yucongalicechen · 2024-12-19T20:09:14Z

@sbillinge Since we're redoing the workflow anyways, I'm adding the skip_config_creation for #244 here too. Here's a new diagram I'm thinking about:

yucongalicechen · 2024-12-20T03:31:15Z

^push an intermediate commit in case anyone would like to comment, I will likely reformat some functions and make the comments clearer

yucongalicechen · 2024-12-20T04:04:13Z

I edited the codes again and it's ready for some feedback now @sbillinge

yucongalicechen · 2024-12-20T04:04:46Z

src/diffpy/utils/tools.py


-def clean_dict(obj):
+
+def _clean_dict(obj):


made this function private

yucongalicechen · 2024-12-20T04:05:39Z

src/diffpy/utils/tools.py

@@ -71,67 +80,64 @@ def load_config(file_path):
 def _sorted_merge(*dicts):
    merged = {}
    for d in dicts:
+        d = _clean_dict(d)


add a step here so we don't need to pass in _clean_dict(d) for every d

yucongalicechen · 2024-12-20T04:06:30Z

src/diffpy/utils/tools.py

+        f"[{user_info.get('username', '')}]:  "
+    ).strip() or user_info.get("username", "")
+    email = input(f"Please enter the your email " f"[{user_info.get('email', '')}]:  ").strip() or user_info.get(
+        "email", ""


name change from args to user_info

yucongalicechen · 2024-12-20T04:09:18Z

src/diffpy/utils/tools.py

    )
-    return return_bool
+    config = {"username": _stringify(username), "email": _stringify(email)}
+    if username and email:


The function originally returns a bool that passes back to get_user_info to see if we need to create a global config file. If user only input none or one of username / email, an empty global config file will be created. I simplified the logic here so that we only create a global config file if both username and email are present. Does this make sense though?

yucongalicechen · 2024-12-20T04:11:31Z

tests/test_tools.py

-    expected_username, expected_email = expected
-    config = get_user_info(args)
+def _run_tests(cli_inputs, expected):
+    user_info = {"username": cli_inputs["cli_username"], "email": cli_inputs["cli_email"]}


"cli_inputs" refer to function arguments passed to get_user_info. Not sure if this is the best name here but I want to distinguish them from user inputs via prompts ("inputs").

Do we want to still have a helper function called _run_tests within each test func?

The name run_tests to me is quite ambiguous - what test is it running? comparing username and email?

Whoops missed this comment. Yeah I think we can keep this test but maybe have a better name

yucongalicechen · 2024-12-20T04:13:01Z

tests/test_tools.py

    confile = Path().home() / "diffpyconfig.json"
-    assert confile.is_file()
+    assert confile.is_file() == expected["config_file_exists"]


global config file should only be created when both username and email are given

yucongalicechen · 2024-12-20T04:14:38Z

tests/test_tools.py

-    inp_iter = iter(inputsb)
-    monkeypatch.setattr("builtins.input", lambda _: next(inp_iter))
-    _run_tests(inputsa, expected)
+    _run_tests(cli_inputs, expected)
    confile = Path().home() / "diffpyconfig.json"
    assert confile.exists() is False


config file is never created if user only provides function arguments

bobleesj · 2024-12-20T06:39:58Z

tests/test_tools.py

+    user_info = {"username": cli_inputs["cli_username"], "email": cli_inputs["cli_email"]}
+    config = get_user_info(user_info=user_info, skip_config_creation=cli_inputs["skip_config_creation"])
+    expected_username = expected["expected_username"]
+    expected_email = expected["expected_email"]
    assert config.get("username") == expected_username
    assert config.get("email") == expected_email


 params_user_info_with_home_conf_file = [


params_user_info_with_home_conf_file may not be needed - please see our discussion: https://github.com/diffpy/diffpy.utils/pull/236/files#r1885496247 @yucongalicechen

I think this is probably a different test case than that issue..?

bobleesj · 2024-12-20T06:47:51Z

tests/test_tools.py

+        {"expected_username": "cli_username", "expected_email": "[email protected]"},
+    ),
+    (
+        {"cli_username": None, "cli_email": "[email protected]", "skip_config_creation": False},


We have 5 variables used - cli_username, cli_email, skip_config, expected_username, expected_email, while we have a lots of repeated key values, making it a bi hard to manage.

can we pass variables to @pytest.mark.parametrize("input_username, input_email, ... "). Also, does it have to be called cli...?

Hey Bob, thanks for the feedback!! I think these repeated key values correspond to different test cases. Basically this function tries to get username/email from four places: (a) prompt input, (b) function arguments (which we called cli input because it was initially used by labpdfproc, where the function arguments are from cli), (c) local config file, (d) global config file, with priority order.

Currently we have four test functions that correspond to:
(1) we only have global config file, function prioritizes (b) to (d) (that is, replace (d) if (b) is non-empty).
(2) we have local config file with/without global config file, function prioritizes (b) to (c).
(3) we dont have config files, and user does not choose to skip config creation, then function prioritizes (a) to (b), and create a global config file if both username/email arguments are present.
(4) we dont have config files, and user chooses to skip config creation (this means we don't have (a)), then the function reads (b), and replace username/email with an empty string if no value given.

So I think all arguments and test values are necessary here. I will add a bit more comment to the tests so that it's clearer. We might be able to find a better way to organize the tests?

Thanks @yucongalicechen for the comments. Yeah I had a quick look at the src code.

Username/email from 4 places:

prompt

function args

local config

global config

and the test functions are basically testing the input/out and reading/creating config files as needed..

Noted.

bobleesj · 2024-12-20T06:49:19Z

tests/test_tools.py

-@pytest.mark.parametrize("inputsa, inputsb, expected", params_user_info_no_conf_file_no_inputs)
-def test_get_user_info_no_conf_file_no_inputs(monkeypatch, inputsa, inputsb, expected, user_filesystem):
+@pytest.mark.parametrize("cli_inputs, expected", params_user_info_no_conf_file_no_inputs)
+def test_get_user_info_no_conf_file_no_inputs(monkeypatch, cli_inputs, expected, user_filesystem):
    _setup_dirs(monkeypatch, user_filesystem)


If you could help me understanding - what's the role of moneypatch and why do we need to setup a dir using _setup_dirs?

this is to set up the global and local file paths for the test cases because the function will try to read files from these filepaths

sbillinge · 2024-12-20T11:57:18Z

I thought about this more and, as I mentioned before, I think it makes more sense to disentangle the two workflows (1) get_user_info and (2) update_config. I sketched this in two images that I paste below. I don't necessarily expect you to be able to read them, but they show the structure.

The starting point of the UC is labpdfproc (LPP), for example, first running an update_global_config workflow, followed by a separate build_metadata workflow, i.e.,

where the build_metadata workflow looks like

The build_metadata workflow is as we have now, runtime > local config > global config for uname, email, orcid.

The update_global_config checks for global_config. If the file is missing or if unam/email/orcid are missing in the file, it runs something like if not skip_config it prompts users for input and creates/updates the config file.

I think separating these makes them more reusable. I want to start using this (and the get_package_info) in all our codes so we start the arduous process of collecting better metadata.

yucongalicechen · 2024-12-20T16:42:09Z

src/diffpy/utils/tools.py

+    return {
+        "username": _stringify(user_info.get("username", "")),
+        "email": _stringify(user_info.get("email", "")),
+    }


simplified the workflow here

yucongalicechen · 2024-12-20T16:48:17Z

@sbillinge I've simplified the workflow using the idea from the diagram, please check.
In the mean time I'll think about how to make the tests more readable as also suggested by @bobleesj (since we'll add orcid this will make the tests extremely long to account for different cases).

yucongalicechen · 2024-12-23T15:27:51Z

replaced by #267

change args to user_info and fix docstring

f1350e5

yucongalicechen commented Dec 19, 2024

View reviewed changes

src/diffpy/utils/tools.py Show resolved Hide resolved

intermediate commit

ca0459b

yucongalicechen mentioned this pull request Dec 20, 2024

implement a skip_config_creation option in get_user_info #250

Closed

reformat and add news

658e1ce

yucongalicechen commented Dec 20, 2024

View reviewed changes

bobleesj reviewed Dec 20, 2024

View reviewed changes

reorganize function workflow

4cfd13e

yucongalicechen commented Dec 20, 2024

View reviewed changes

sbillinge mentioned this pull request Dec 22, 2024

refactor to separate getting info and creating config files #264

Merged

yucongalicechen closed this Dec 23, 2024

yucongalicechen deleted the user-info branch December 23, 2024 15:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

change args to user_info and fix docstring #253

change args to user_info and fix docstring #253

yucongalicechen commented Dec 19, 2024 •

edited

Loading

codecov bot commented Dec 19, 2024 •

edited

Loading

sbillinge commented Dec 19, 2024

yucongalicechen commented Dec 19, 2024

yucongalicechen commented Dec 19, 2024

yucongalicechen commented Dec 20, 2024

yucongalicechen commented Dec 20, 2024

yucongalicechen Dec 20, 2024 •

edited

Loading

yucongalicechen Dec 20, 2024 •

edited

Loading

yucongalicechen Dec 20, 2024

yucongalicechen Dec 20, 2024

yucongalicechen Dec 20, 2024

bobleesj Dec 20, 2024

yucongalicechen Dec 20, 2024

yucongalicechen Dec 20, 2024

yucongalicechen Dec 20, 2024

bobleesj Dec 20, 2024

yucongalicechen Dec 20, 2024

bobleesj Dec 20, 2024

yucongalicechen Dec 20, 2024

bobleesj Dec 20, 2024

bobleesj Dec 20, 2024

yucongalicechen Dec 20, 2024

sbillinge commented Dec 20, 2024 •

edited

Loading

yucongalicechen Dec 20, 2024

yucongalicechen commented Dec 20, 2024

yucongalicechen commented Dec 23, 2024

change args to user_info and fix docstring #253

change args to user_info and fix docstring #253

Conversation

yucongalicechen commented Dec 19, 2024 • edited Loading

codecov bot commented Dec 19, 2024 • edited Loading

Codecov Report

sbillinge commented Dec 19, 2024

yucongalicechen commented Dec 19, 2024

yucongalicechen commented Dec 19, 2024

yucongalicechen commented Dec 20, 2024

yucongalicechen commented Dec 20, 2024

yucongalicechen Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

yucongalicechen Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbillinge commented Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

yucongalicechen commented Dec 20, 2024

yucongalicechen commented Dec 23, 2024

yucongalicechen commented Dec 19, 2024 •

edited

Loading

codecov bot commented Dec 19, 2024 •

edited

Loading

yucongalicechen Dec 20, 2024 •

edited

Loading

yucongalicechen Dec 20, 2024 •

edited

Loading

sbillinge commented Dec 20, 2024 •

edited

Loading