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

Add generated dates (#864) #877

Merged
merged 6 commits into from
Jul 25, 2018
Merged

Add generated dates (#864) #877

merged 6 commits into from
Jul 25, 2018

Conversation

beckjake
Copy link
Contributor

Add a field named generated_at to the root of the catalog and the manifest, which contains an RFC3339-formatted timestamp.

@beckjake beckjake requested a review from cmcarthur July 24, 2018 19:52
Copy link
Member

@cmcarthur cmcarthur left a comment

Choose a reason for hiding this comment

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

you need to change one json schema. then, if the tests pass, this will be approved.

@@ -252,6 +252,9 @@
'properties': {
'nodes': PARSED_NODES_CONTRACT,
'macros': PARSED_MACROS_CONTRACT,
'generated_at': {
'type': 'date-time'
Copy link
Member

Choose a reason for hiding this comment

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

the correct json schema draft4 type for this is:

{
    "type": "string",
    "format": "date-time"
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, I was wondering how tests were passing after this comment - it turns out we never use this schema at all. Should I just remove it entirely?

Copy link
Member

Choose a reason for hiding this comment

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

we need at least one json schema for the manifest, and at least one for the catalog. it's actually really important as a piece of documentation to share with the community. so ideally we'd either (a) apply it or (b) just leave it in here but do our best to correct it, even though it's not currently used. up to you which you want to do here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've changed the ParsedManifest to validate the schema (and made sure it fails on the old schema).

self.assertGreater(
now, parsed,
'parsed date {} happened in the future'.format(parsed)
)
Copy link
Member

Choose a reason for hiding this comment

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

hah interesting note about freezegun.freeze_time. i think this is fine as a single-use hack. let's make sure not to overuse this.

Copy link
Member

@cmcarthur cmcarthur left a comment

Choose a reason for hiding this comment

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

appveyor :(

datetime.datetime(2018, 7, 24, 21, 29, 49, 490799) not greater than datetime.datetime(2018, 7, 24, 21, 29, 49, 490799)

these code changes look excellent, approved from my end. just fix that appveyor issue.

@beckjake beckjake merged commit c7c3d09 into dev/isaac-asimov Jul 25, 2018
@beckjake beckjake deleted the add-generated-dates branch July 25, 2018 14:04
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.

2 participants