Skip to content

Added extra information in warning for better capturing of user info #149

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.

Sign up for GitHub

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

Merged
merged 9 commits into from
Nov 7, 2024
Merged

Added extra information in warning for better capturing of user info #149

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
7112fac
Add warning to missing global config file
Nov 5, 2024
d24acb6
fix line too long
Nov 5, 2024
0a59a0b
better wording for capture user info
Nov 5, 2024
2379a62
[pre-commit.ci] auto fixes from pre-commit hooks
pre-commit-ci[bot] Nov 5, 2024
4bb5ecf
delete unnecessary get-user-fix.rst
Nov 6, 2024
2732b49
edited get user prompt
Nov 6, 2024
e36726e
edited message and prompt to capture user info
Nov 6, 2024
c367349
[pre-commit.ci] auto fixes from pre-commit hooks
pre-commit-ci[bot] Nov 6, 2024
5bfc9dd
fixed pre-commit issues
Nov 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions news/capture-user.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
**Added:**

* Better wording on the capture user info functionality

**Changed:**

* <news item>

**Deprecated:**

* <news item>

**Removed:**

* <news item>

**Fixed:**

* <news item>

**Security:**

* <news item>
23 changes: 23 additions & 0 deletions news/get-user-fix.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
**Added:**
Copy link
Contributor

Choose a reason for hiding this comment

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

we can delete this file.


* Warning message for missing global config file

**Changed:**

* <news item>

**Deprecated:**

* <news item>

**Removed:**

* <news item>

**Fixed:**

* <news item>

**Security:**

* <news item>
6 changes: 6 additions & 0 deletions src/diffpy/utils/tools.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import importlib.metadata
import json
import os
import warnings
from copy import copy
from pathlib import Path

Expand Down Expand Up @@ -114,6 +115,11 @@ def get_user_info(args=None):
global_config = load_config(Path().home() / "diffpyconfig.json")
local_config = load_config(Path().cwd() / "diffpyconfig.json")
if global_config is None and local_config is None:
warnings.warn(
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not quite what I had in mind. Just the warning messages in the prompts could be clearer. We could put this preamble message here if we really like it, but I would like to see it in the context of the full workflow pasted in the comments before deciding.

@alisnwu have you run through the whole workflow to see what it looks like and where it can be improved?

Even if we do want this wording, I think this is not a good use of warnings.warn. We are not using any of its features. A simple print statement would be clearer.

Copy link
Author

Choose a reason for hiding this comment

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

@sbillinge Would this be a better way to word the prompts?

Screenshot 2024-11-06 at 10 42 10 AM

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, I like that (modulo a slight grammatical boo boo)!

How about something a bit more human friendly:
"No global configuration file was found containing information about the user to associate with the data. By following the prompts below you can add your name and email to this file on the current computer and your name will be automatically associated with subsequent diffpy data by default. This is not recommended on a shared or public computer. You will only have to do that once. For more information "

we need a workflow for people to update this global config file later if they want. Pease could you make an issue to check/work on that and maybe do it on a separate PR? Thanks!

"No global config file, please follow prompts below. "
"The global config file is very important in crediting your work in the future. "
"For more information, please refer to www.diffpy.org/diffpy.utils/examples/toolsexample.html"
)
config_bool = _create_global_config(args)
global_config = load_config(Path().home() / "diffpyconfig.json")
config = _sorted_merge(clean_dict(global_config), clean_dict(local_config), clean_dict(args))
Expand Down