Uh oh!
There was an error while loading. Please reload this page.
Added extra information in warning for better capturing of user info - #149
Conversation
sbillinge
left a comment
There was a problem hiding this comment.
maybe the best way to do this is to paste the outputs into the comments under different situations and we can discuss what is the clearest wording?
| @@ -0,0 +1,23 @@ | |||
| **Added:** | |||
| 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( |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@sbillinge Would this be a better way to word the prompts?
There was a problem hiding this comment.
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!
alisnwu
commented
Nov 6, 2024
 @sbillinge ready for review |
sbillinge
commented
Nov 6, 2024
Thanks @alisnwu It seems to be failing pre-commit. Also, I think it would be easier to read if it were formatted a bit better on the terminal. This will depend on the configuration on the user's computer but having some line-breaks and/or empty lines to make it easier to read would be nice I think? What do you think? Please can you check with Bob why the tests are not running? |
bobleesj
commented
Nov 6, 2024
@alisnwu Once you are done with formatting, could you tag me? Let's see if the tests are running. Also, plz don't forget to run Ref: https://github.com/bobleesj/command-line-cheatsheet/?tab=readme-ov-file#command-line-shortcuts |
alisnwu
commented
Nov 6, 2024
 @sbillinge I added some newline to make the message easier to read, how does this look? |
bobleesj
commented
Nov 7, 2024
@sbillinge#159 CI Pytest running again - need to merge this to see the global effect |
sbillinge
commented
Nov 7, 2024
nice! Thanks @alisnwu ! |
closes#129 and #140
@sbillinge ready for review