Create time-stamp and load it into args #60
Conversation
There was a problem hiding this comment.
This is great! please see inline comments.
A few other thoughts:
- it is failing tests, we need to add freezegun to the developer requirements file
- it is failing precommit...please make sure it passes pre-commit before pushing. Have you run
pre-commitinstall on your local? If so I am not sure what is wrong, but something is. - small workflow tweaks: please make the PR title just describe the main gist of the PR in less than 80 chars, then put more details below in the first comment box. Also, please add the
Closes #in that first comment box with the issue number it closes. - your commit message is very good now! congrats on that. In general I would expect more than one commit for this amount of work. For example a separate commit for the test and then for the code. this allows us to rewind keeping the test but redoing the code, for example. tl;dr, make slightly more frequent commits is recommended.
But in general, this is fantastic, very high quality work!
src/diffpy/labpdfproc/tools.py
Outdated
| ------- | ||
| the updated argparse Namespace with datetime inserted as key-value pairs | ||
| """ | ||
| if args.user_metadata: |
There was a problem hiding this comment.
this is not user metadata, so we don't need this if statement. We will create the time-stamp regardless.
Let's think about the name a bit too. datetime is a type not really a thing. maybe created_at or creation_time. What do you think? This will appear in file headers for future users to read so we would like the key names to be as descriptive as possible.
There was a problem hiding this comment.
Yes, I get your point. I would follow your instructions to change it. Also, I think using time-stamp is a good idea. Maybe we could change to form of creation_time as you think as a file headers. Additionally, I'm resetting pre-commit since I found out that it did not truly set up after installing it.
There was a problem hiding this comment.
That's great.
Our design is to load into args whatever we want in the resulting file-header, then the dump will just dump whatever is in the args into the header. This is easier to maintain, but it means we really don't want to change the name of a key at dump-time, so I would advocate for calling it creation_time also in the args.
stevenhua0320
changed the title
stevenhua0320
changed the title
sbillinge
changed the title
|
I did the PR title and comment as I would have done it as an example. I think you didn't quite understand my comment in the review on this point, but hopefully doing it this way clearer. All good, I just want to model good practices early...a gift that keeps on giving.... |
OK, Professor, now I know about how to format the structure in the title, comment, and commit. |
…bute the same as expected
There was a problem hiding this comment.
Thanks for this Steven. We completed the use-cases succesfully, and there was some discussion about code design, but none of the designs called for loading a time at the command line, so this is not needed. To make sure we are on the same page,, maybe it is a good idea if you summarize in that issue for each of the UCs we will implement (basically UC5 and there may have been one also from @yucongalicechen , I would have to check again) your strategy for implementing it (which functions etc. you need to write and where/how they will be called). It can also be a useful process to quickly write pseudocode (it is plain language but written in a structure as if were code) for each of the tests you will need.
I think we should close this PR and only when we finish that other planning process, make a clean branch on a fully synced (at that time) main and reapply the --is-test which I think is the only part of this that we will need to keep.
Thanks so much, this is a learning process so please don't get discouraged. I hope the planning process can be quick but it may take a few iterations, so to get it done within a day we all need to try and respond as quickly as we can.
| params7 = [(["--user-metadata", "facility=NSLS II", "beamline=28ID-2", "favorite color=blue"], | ||
| [["facility", "NSLS II"], ["beamline", "28ID-2"], ["favorite color", "blue"], | ||
| ["datetime", "2014-02-01 12:34:56"]])] | ||
| params7 = [(["--creation_time", "2014-02-01 12:34:56"], |
There was a problem hiding this comment.
we don't want --creation-time as an input. Please delete.
| actual_args = load_user_metadata(time_loaded_args) | ||
| assert actual_args == expected | ||
| actual_args = ["2.5", "data.xy"] + inputs | ||
| actual_args = load_datetime(actual_args) |
There was a problem hiding this comment.
we won't be loading a datetime so this function is not needed.
closes #29
Set a --is-test in get_args function.
Also we use freezegun to freeze time in test.