load username and email #63

yucongalicechen · 2024-05-20T14:24:15Z

closes load the username into the metadata #30, closes load user email into metadata #31.
Initial commit on UC1 and UC4 in load the username into the metadata #30. Changed the pseudocode structued in that issue to add the functionality of overwriting config file using user inputs. For tests I added two json config files in conftest.py.
Added user_config.py to manage user configuration.
Added load_user_info function in labpdfprocapp.py right after get_args().
I'll address UC5 later. For the case of multiple usernames and emails, I haven't thought of a great way of doing it now..

…en yet

sbillinge

thanks for this @yucongalicechen .

When you write hte tests I will review those and then I will look at the code.

…e data with user inputs

sbillinge

please see inline

src/diffpy/labpdfproc/tests/conftest.py

sbillinge · 2024-05-21T13:27:04Z

src/diffpy/labpdfproc/tests/conftest.py

@@ -44,4 +47,10 @@ def user_filesystem(tmp_path):
        f.write("good_data.xy \n")
        f.write(f"{str(input_dir.resolve() / 'good_data.txt')}\n")

+    user_config_data = {"username": "good_username", "email": "[email protected]"}
+    with open(conf_dir / "test.json", "w") as f:


I think this needs to have the correct name here and below... diffpyconf.json

sbillinge · 2024-05-21T13:29:01Z

src/diffpy/labpdfproc/tests/conftest.py

+    user_config_data = {"username": "good_username", "email": "[email protected]"}
+    with open(conf_dir / "test.json", "w") as f:
+        json.dump(user_config_data, f)
+    with open(conf_dir / "test2.json", "w") as f:


this is not allowed. We will only check for diffpyconf.json.

However, we will check for it here, but also in cwd where the programming is running. We would like to test both situations so we can't write that second file in conftest.py but will have to write it in a bespoke test.

sbillinge · 2024-05-21T13:36:14Z

src/diffpy/labpdfproc/tests/test_tools.py

+def test_load_user_info(monkeypatch, inputs, expected, user_filesystem):
+    os.chdir(user_filesystem / "conf_dir")
+    expected_username, expected_email = expected
+    input_username, input_email, input_config_file = inputs


we are not inputting a config file. options are:

find diffpyconfig.json in cwd

failing that, find diffpyconfig.json in ~

failing that, check if username and email are provided in the input line

failing that, trigger the workflow to create diffpyconfig.json in ~

Then the workflow is:

If 1 or 3, check that those files contain username/email. If they do continue, if they don't trigger workflow to update diffpyconfig.json

If continue above, then check if username/email are provided at the command line. If yes, use them (but don't update config). If not, use the values in config.

if (4.) above, then use these values to create the diffpyconfig.json

I edited the tests. I included "paths" in @pytest.mark.parametrize because it helps manage different cwd and home directories, not sure if this is the best approach though.

sbillinge

This testing structure is hard for me to understand so I am not sure if it is capturing the right information. It may be ok, but not easy to follow.

I was thinking something more like

def test_load_user_info_configfile(monkeypatch, inputs, expected, user_filesystem):
    os.chdir(user_filesystem)

    # test it works when config file is in home directory
    cli_inputs = ["2.5", "data.xy"]
    actual_args = get_args(cli_inputs)
    actual_args = load_user_info(actual_args)
    assert actual_args.username == expected_args_username
    assert actual_args.email == expected_args_email

    # test it works when config file is in current directory
    with open(user_filesystem / 'diffpyconfig.json', "w") as f:
        config_data = json.dump(user_data f)
    cli_inputs = ["2.5", "data.xy"]
    actual_args = get_args(cli_inputs)
    actual_args = load_user_info(actual_args)
    assert actual_args.username == expected_args_username
    assert actual_args.email == expected_args_email

then other tests for other cases

this seems more understandable to me, though your tests may also be working, I just can't follow them too easily.

yucongalicechen · 2024-05-21T22:32:40Z

Okay, yes I also think this is more readable. But this is not testing the config file? I was using different paths because I wanted to test if the config file was modified or not by the inputs.

sbillinge · 2024-05-21T22:52:29Z

I see. I would put the different behaviors in different tests. We usually use one test to test one behavior.

…

On Tue, May 21, 2024 at 6:33 PM Yucong (Alice) Chen < ***@***.***> wrote: Okay, yes I also think this is more readable. But this is not testing the config file? I was using different paths because I wanted to test if the config file was modified or not by the inputs. — Reply to this email directly, view it on GitHub <#63 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABAOWUI2XD6VFIEOTYGMNOLZDPDR3AVCNFSM6AAAAABH72HJPWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMRTGU2DEMJUHA> . You are receiving this because you commented.Message ID: <diffpy/diffpy. ***@***.***>

-- Simon Billinge Professor, Department of Applied Physics and Applied Mathematics Columbia University

sbillinge · 2024-05-21T22:56:08Z

what I mean by that is in this test there is a valid config file that doesn't need to be updated.....but it can still be overloaded by a command line input

…

On Tue, May 21, 2024 at 6:52 PM Simon Billinge ***@***.***> wrote: I see. I would put the different behaviors in different tests. We usually use one test to test one behavior. On Tue, May 21, 2024 at 6:33 PM Yucong (Alice) Chen < ***@***.***> wrote: > Okay, yes I also think this is more readable. But this is not testing the > config file? I was using different paths because I wanted to test if the > config file was modified or not by the inputs. > > — > Reply to this email directly, view it on GitHub > <#63 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/ABAOWUI2XD6VFIEOTYGMNOLZDPDR3AVCNFSM6AAAAABH72HJPWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMRTGU2DEMJUHA> > . > You are receiving this because you commented.Message ID: <diffpy/diffpy. > ***@***.***> > -- Simon Billinge Professor, Department of Applied Physics and Applied Mathematics Columbia University

-- Simon Billinge Professor, Department of Applied Physics and Applied Mathematics Columbia University

yucongalicechen · 2024-05-21T23:09:39Z

Yes I get it. It's just my previous codes updated the config files, so I wanted to correct that. The tests are a bit similar so I was thinking of ways to put them together. I now separate the tests into no config files, config file in home directory, and config file in cwd.

sbillinge

I think we can test everything (except the missing config file) in one test thinking about it. We just have to test the username and email come out right when the right flow is followed, then we read the config file in conf_dir and make sure it has the right thing in it.

For the missing config file we will have to delete the config file in home before running the same test. We can call the user home_dir_user, cwd_user, cli_user, input_user and same for the emails and make sure the right user shows up in args and in the home_dir/diffpyconf.json These can be handed in in params

What do you think?

sbillinge · 2024-05-22T00:10:25Z

def test_load_user_info_configfile(monkeypatch, inputs, expected, user_filesystem):
    os.chdir(user_filesystem)

    # test it works when config file is in home directory
    cli_inputs = ["2.5", "data.xy"]
    actual_args = get_args(cli_inputs)
    actual_args = load_user_info(actual_args)
    assert actual_args.username == expected_args_username
    assert actual_args.email == expected_args_email

    # test it works when config file is in current directory
    with open(user_filesystem / 'diffpyconfig.json', "w") as f:
        config_data = json.dump(user_data f)
    cli_inputs = ["2.5", "data.xy"]
    actual_args = get_args(cli_inputs)
    actual_args = load_user_info(actual_args)
    assert actual_args.username == expected_args_username
    assert actual_args.email == expected_args_email

yucongalicechen · 2024-05-22T00:26:31Z

Yes I think we can combine these. Since this PR is closed, shall I make another PR for the updates?

sbillinge · 2024-05-22T00:40:46Z

No you can use this PR, I didn't know it was closed

…

On Tue, May 21, 2024, 8:26 PM Yucong (Alice) Chen ***@***.***> wrote: Yes I think we can combine these. Since this PR is closed, shall I make another PR for the updates? — Reply to this email directly, view it on GitHub <#63 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABAOWUON6ZRWUG4MAM6E7C3ZDPQ4ZAVCNFSM6AAAAABH72HJPWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMRTGYZTKNZXGY> . You are receiving this because you modified the open/close state.Message ID: ***@***.***>

sbillinge · 2024-05-22T11:39:02Z

I see I closed it, but that was in error, I thought it was a stale PR from earlier. My apologies.

I think writing a little test private function that asserts that the name and email are correct in args and also that they are correct in the home_dir/diffpyconfig.json will do it, then this function can be called with all the different arrangements of things on disc (config files present/absent/valid/invalid in different locations).

Actually, I thought more about this and I think that this is a capability that we may want to have working across many our diffpy programs (which is why I chose the name of the config file with diffpy not labpdfproc), but from this point of view, having this capability in diffpy.utils rather than labpdfproc actually makes sense to me. We can maybe finish the reviewing process here on this PR but not merge it, then copy the code over to diffpy.utils and merge it over there?

I think this is also true for the time-stamp and package name-version capabilities. Everywhere we write .chi we would like them to look like this.

yucongalicechen · 2024-05-22T12:39:43Z

Ah, okay, got it. I'll push my latest changes here and copy over the codes to diffpy.utils.

sbillinge

couple of quick comments. But are you then going to combine tests into one or two test functions as we discussed? Please reach out if you have questions.

sbillinge · 2024-05-22T17:11:40Z

src/diffpy/labpdfproc/tests/conftest.py

@@ -8,6 +9,8 @@ def user_filesystem(tmp_path):
    base_dir = Path(tmp_path)
    input_dir = base_dir / "input_dir"
    input_dir.mkdir(parents=True, exist_ok=True)
+    conf_dir = base_dir / "conf_dir"


let's call it home_dir instead of conf_dir to make the intent clearer.

sbillinge · 2024-05-22T17:13:27Z

src/diffpy/labpdfproc/tests/conftest.py

@@ -44,4 +47,8 @@ def user_filesystem(tmp_path):
        f.write("good_data.xy \n")
        f.write(f"{str(input_dir.resolve() / 'good_data.txt')}\n")

+    user_config_data = {"username": "good_username", "email": "[email protected]"}


I think this should be home_user and [email protected]

yucongalicechen · 2024-06-11T17:17:22Z

replaced by #77

initial commit: added user_config and load_user_info, tests not writt…

33a0bd2

…en yet

sbillinge reviewed May 20, 2024

View reviewed changes

yucongalicechen added 3 commits May 21, 2024 09:16

added tests for UC1 and UC4, rewrite function to overwrite config fil…

c2a838e

…e data with user inputs

changed config file name to diffpyconfig

3967018

corrected file path for bad test cases

b19f8b8

sbillinge reviewed May 21, 2024

View reviewed changes

updated tests and functions according to workflow

d8b411c

sbillinge reviewed May 21, 2024

View reviewed changes

reformatted tests

bfbb4bb

sbillinge reviewed May 21, 2024

View reviewed changes

sbillinge closed this May 22, 2024

yucongalicechen reopened this May 22, 2024

reformatted tests to with/without config file

5519c54

sbillinge reviewed May 22, 2024

View reviewed changes

yucongalicechen closed this Jun 11, 2024

yucongalicechen deleted the user branch June 14, 2024 13:38

load username and email #63

load username and email #63

Uh oh!

Conversation

yucongalicechen commented May 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sbillinge left a comment

Choose a reason for hiding this comment

Uh oh!

sbillinge left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sbillinge May 21, 2024

Choose a reason for hiding this comment

Uh oh!

sbillinge May 21, 2024

Choose a reason for hiding this comment

Uh oh!

sbillinge May 21, 2024

Choose a reason for hiding this comment

Uh oh!

yucongalicechen May 21, 2024

Choose a reason for hiding this comment

Uh oh!

sbillinge left a comment • 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!

yucongalicechen commented May 21, 2024

Uh oh!

sbillinge commented May 21, 2024 via email

Uh oh!

sbillinge commented May 21, 2024 via email

Uh oh!

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

Uh oh!

Uh oh!

sbillinge left a comment • 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!

sbillinge commented May 22, 2024

Uh oh!

yucongalicechen commented May 22, 2024

Uh oh!

sbillinge commented May 22, 2024 via email

Uh oh!

sbillinge commented May 22, 2024

Uh oh!

yucongalicechen commented May 22, 2024

Uh oh!

sbillinge left a comment

Choose a reason for hiding this comment

Uh oh!

sbillinge May 22, 2024

Choose a reason for hiding this comment

Uh oh!

sbillinge May 22, 2024

Choose a reason for hiding this comment

Uh oh!

yucongalicechen commented Jun 11, 2024

Uh oh!

Uh oh!

yucongalicechen commented May 20, 2024 •

edited

Loading

sbillinge left a comment •

edited

Loading

yucongalicechen commented May 21, 2024 •

edited

Loading

sbillinge left a comment •

edited

Loading