Skip to content
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.

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

Fix timestamp / -> - #2075

Merged
merged 6 commits into from
Feb 15, 2017
Merged

Fix timestamp / -> - #2075

merged 6 commits into from
Feb 15, 2017

Conversation

antgonza
Copy link
Member

@antgonza antgonza commented Feb 9, 2017

@wasade and @josenavas this is what we discussed during the last meeting about the new timestamp formats.

@josenavas
Copy link
Contributor

@antgonza unfortunately changing / to - are not enough since the actual proposed change also involved changing the order of the data: %m/%d/%Y %H:%M:%S -> %Y-%m-%d %H:%M:%S

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 91.555% when pulling c8113ea on antgonza:fix-timestamp into 53188a6 on biocore:master.

@antgonza
Copy link
Member Author

This is ready for review: @josenavas, @wasade, @ElDeveloper

A lot of the changes were to update the tests to match the new date formats. Remember that now we are actually patching the test DB/files so this change also implies that I needed to change how we are testing some functionality. However, I think there are not big changes.

Copy link
Contributor

@wasade wasade left a comment

Choose a reason for hiding this comment

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

Just some comments. A few blocks were a bit confusing

break
except ValueError:
pass
if date is not None:
Copy link
Contributor

Choose a reason for hiding this comment

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

this block is rather confusing...

Copy link
Member Author

Choose a reason for hiding this comment

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

Any suggestions? Basically, is testing which is the fmt of the date within the old formats, if it's found it using the new format. Now, new format doesn't accept 2 digits so I'm simply reassigning to one with 4 digits if one of the 2 year digits is found ...

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah. What about a format mapping: {old_fmt: new_format}, and in the case of unacceptable, just specify None as the new format?

fn_prep = [f for f in tgz_obs
if f.startswith('templates/1_prep_1_')][0]
# 3 times
self.assertTrue(fn_prep in tgz_obs)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why would the item continue to exist after being removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

because we created the tests we added more artifacts and didn't assign new file paths so we reused the same ones

| | or ``mm/dd/yyyy hh`` | |
| | or ``mm/dd/yyyy`` | |
| | or ``mm/yyyy`` | |
| ``collection_timestamp`` | ``yyyy-mm-dd hh:mm:ss`` | The time stamp (preferred) of when the sample was collected. Several format are accepted, all ISO 8601. |
Copy link
Contributor

Choose a reason for hiding this comment

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

timezone or are these UTC?

Copy link
Member Author

Choose a reason for hiding this comment

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

User defined? We never got to discuss this in the meetings, do you, @josenavas have a preference?

Copy link
Contributor

Choose a reason for hiding this comment

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

No preference, whatever the standard says.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 91.554% when pulling f4835d5 on antgonza:fix-timestamp into 53188a6 on biocore:master.

| | or ``mm/dd/yyyy hh`` | |
| | or ``mm/dd/yyyy`` | |
| | or ``mm/yyyy`` | |
| ``collection_timestamp`` | ``yyyy-mm-dd hh:mm:ss`` | The time stamp (preferred) of when the sample was collected. Several format are accepted, all ISO 8601. |
Copy link
Contributor

Choose a reason for hiding this comment

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

No preference, whatever the standard says.

@wasade wasade merged commit aa68a21 into qiita-spots:master Feb 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants