refactor to separate getting info and creating config files #264

sbillinge · 2024-12-21T21:11:35Z

closes #245
replaces #253

@yucongalicechen @bobleesj ready for review. I will put the config-file creation workflow on another PR

Commented text will be reused (and removed from here) in a separate function for creating a missing global config file in a future PR.

codecov · 2024-12-21T21:25:49Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.68%. Comparing base (9bd388d) to head (c2ac173).
Report is 28 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff             @@
##              main     #264      +/-   ##
===========================================
- Coverage   100.00%   98.68%   -1.32%     
===========================================
  Files            8        8              
  Lines          391      381      -10     
===========================================
- Hits           391      376      -15     
- Misses           0        5       +5

Files with missing lines	Coverage Δ
tests/conftest.py	`100.00% <100.00%> (ø)`
tests/test_tools.py	`86.11% <100.00%> (-13.89%)`	⬇️

... and 2 files with indirect coverage changes

bobleesj · 2024-12-22T01:43:39Z

doc/source/examples/tools_example.rst

@@ -40,16 +41,17 @@ The function will return

 .. code-block:: python

-        {"email": "janedoe@email.com", "username": "Jane Doe"}
+        {"owner_email": "janedoe@email.com", "owner_name": "Jane Doe", "owner_orcid": "0000-0000-0000-0000"}


i like the term "owner" - and orcid since that's a gateway for more info

bobleesj · 2024-12-22T01:44:30Z

doc/source/examples/tools_example.rst

-It looks first for the file in the current working directory.  If it cannot find it there it will look
-user's home, i.e., login, directory.  To find this directory, open a terminal and a unix or mac system type ::
+It looks for files in the current working directory and in the computer-user's home (i.e., login) directory.
+For example, it might be in C:/Users/yourname`` or something like that, but to find this directory, open


much clearer since there is an example provided since i wasn't sure what "login" meant here.

bobleesj · 2024-12-22T01:45:27Z

doc/source/examples/tools_example.rst

-Getting user data with no config files and with no interruption of execution
----------------------------------------------------------------------------
+If no configuration files can be found, they can be created using a text editor, or by using a diffpy tool
+called ``check_and_build_global_config()`` which, if no global config file can be found, prompts the user for the


clear - when no config is found, this function is called.

bobleesj · 2024-12-22T01:46:51Z

doc/source/examples/tools_example.rst

+``check_and_build_global_config()`` takes an optional boolean ``skip_config_creation`` parameter that
+could be set to ``True`` at runtime to override the config creation.
+
+I entered the wrong information in my config file so it always loads incorrect information, how do I fix that?


Clear on how to reset config

bobleesj · 2024-12-22T01:48:08Z

src/diffpy/utils/tools.py

@@ -91,47 +90,62 @@ def _create_global_config(args):
    return return_bool


-def get_user_info(args=None):
+def get_user_info(owner_name=None, owner_email=None, owner_orcid=None):


much better - instead of args, this line explicitly communicates what info can be optionally provided

bobleesj · 2024-12-22T01:48:58Z

src/diffpy/utils/tools.py


    Parameters
    ----------
-    args argparse.Namespace
-        The arguments from the parser, default is None.
+    owner_name: string, optional, default is the value stored in the global or local config file.


Ah.. this gives me joy

bobleesj · 2024-12-22T01:52:14Z

tests/test_tools.py

-    local_config_data = {"username": "cwd_username", "email": "cwd@email.com"}
-    with open(Path(user_filesystem) / "diffpyconfig.json", "w") as f:
+@pytest.mark.parametrize(
+    "runtime_inputs, expected",


i like it - runtime is much clearer than cli

bobleesj · 2024-12-22T01:53:31Z

tests/test_tools.py

-@pytest.mark.parametrize("inputs, expected", params_user_info_with_home_conf_file)
-def test_get_user_info_with_home_conf_file(monkeypatch, inputs, expected, user_filesystem):
-    _setup_dirs(monkeypatch, user_filesystem)
-    _run_tests(inputs, expected)


yes, this mysterious _run_tests func is replaced.

bobleesj · 2024-12-22T01:53:59Z

tests/test_tools.py

+    ],
+)
+def test_get_user_info_with_home_conf_file(runtime_inputs, expected, user_filesystem, mocker):
+    # user_filesystem[0] is tmp_dir/home_dir with the global config file in it, user_filesystem[1]


i was wondering user_filesystem and the comment clarifies it

bobleesj · 2024-12-22T01:56:19Z

@sbillinge I did my review via in-line comments.

One general question, should we follow pep8 convention for comments? 1) capitalize the first word

PEP8 Comments should be complete sentences. The first word should be capitalized, unless it is an identifier that begins with a lower case letter (never alter the case of identifiers!).

REF https://peps.python.org/pep-0008/#comments

sbillinge · 2024-12-22T12:07:44Z

@yucongalicechen @bobleesj I need the updated user_filesystem for the other PR on the config updater so I will merge this to get it. i think it is in pretty good shape (thanks @bobleesj for the feedback). @yucongalicechen please take a look at this PR when you get a chance and if there is anything you find needs changing we can do it on another PR.

yucongalicechen · 2024-12-22T15:20:33Z

src/diffpy/utils/tools.py

@@ -65,7 +64,7 @@ def load_config(file_path):
            config = json.load(f)
        return config
    else:
-        return None
+        return {}


 def _sorted_merge(*dicts):


I wonder do we want to make some of the functions private (like load_config)?

yes, this should probably be private, I agree.

Can we also check? I don't think we need _sorted_dict() any more so that could be deleted on a new PR.

Yes, I can fix these on a new PR, once that updater workflow PR is merged

yucongalicechen · 2024-12-22T15:21:12Z

src/diffpy/utils/tools.py

+        The name of the user who will show as owner in the metadata that is stored with the data
+    owner_email: string, optional, default is the value stored in the global or local config file.
+        The email of the user/owner
+    owner_name:  string, optional, default is the value stored in the global or local config file.


typo - owner_orcid

thanks, good catch.

yucongalicechen · 2024-12-22T15:24:58Z

doc/source/examples/tools_example.rst

+
+It is easy to fix this simply by deleting the global and/or local config files, which will allow
+you to re-enter the information during the ``check_and_build_global_config()`` initialization
+workflow.   You can also simply editi the ``diffpyconfig.json`` file directly using a text


typo - edit

yucongalicechen · 2024-12-22T15:26:30Z

I finished my review @sbillinge here. Like this new structure and just caught two typos.

sbillinge · 2024-12-22T15:47:06Z

Thanks for the excellent review @yucongalicechen . Please could you make a PR to fix those things? Let's also double check there are no private functions that are not use, or public functions that we want to make private.

If @bobleesj finds anything else we can put that into the same PR.

I also wanted to add into the docs what happens in the update config workflow when it is triggered, so reproduce what the user would see and give an example of how they would respond. It would be great for me if that could be in the same PR?

yucongalicechen · 2024-12-22T15:50:48Z

Thanks for the excellent review @yucongalicechen . Please could you make a PR to fix those things? Let's also double check there are no private functions that are not use, or public functions that we want to make private.

If @bobleesj finds anything else we can put that into the same PR.

I also wanted to add into the docs what happens in the update config workflow when it is triggered, so reproduce what the user would see and give an example of how they would respond. It would be great for me if that could be in the same PR?

Sounds good, I'll do that

sbillinge added 2 commits December 21, 2024 16:08

refactor to separate getting info and creating config files

ab0fc8a

commenting out broken tests that will be fixed in make_config PR

b819f70

docs

c2ac173

bobleesj reviewed Dec 22, 2024

View reviewed changes

sbillinge merged commit d791c63 into main Dec 22, 2024
4 of 5 checks passed

sbillinge deleted the userinfo branch December 22, 2024 12:07

yucongalicechen reviewed Dec 22, 2024

View reviewed changes

yucongalicechen mentioned this pull request Dec 22, 2024

check that the name `get_user_info/data is used consistently in docs #256

Closed

yucongalicechen mentioned this pull request Dec 22, 2024

fix typo, remove unncessary functions, and add example for update_config_workflow #268

Closed

refactor to separate getting info and creating config files #264

refactor to separate getting info and creating config files #264

Uh oh!

Conversation

sbillinge commented Dec 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Dec 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bobleesj Dec 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bobleesj commented Dec 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sbillinge commented Dec 22, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yucongalicechen commented Dec 22, 2024

Uh oh!

sbillinge commented Dec 22, 2024

Uh oh!

yucongalicechen commented Dec 22, 2024

Uh oh!

Uh oh!

sbillinge commented Dec 21, 2024 •

edited

Loading

codecov bot commented Dec 21, 2024 •

edited

Loading

bobleesj Dec 22, 2024 •

edited

Loading

bobleesj commented Dec 22, 2024 •

edited

Loading