Skip to content

add timestamps extension#161

Merged
lossyrob merged 5 commits into
developfrom
sjk/timestamps-extension
Aug 20, 2020
Merged

add timestamps extension#161
lossyrob merged 5 commits into
developfrom
sjk/timestamps-extension

Conversation

@simonkassel
Copy link
Copy Markdown
Contributor

adds timestamps extension

closes #135

One question I have is about the apply method in TimestampsItemExt (and this question could be relevant to other extensions as well): if you pass null values for optional arguments defining properties that are already defined in that item, should the null value override the existing property?

So for example:

item already has published and expired defined.

if i do item.ext.timestamps.apply(unpublished=d), this will convert those other two properties to None. Should this be the case?

@lossyrob
Copy link
Copy Markdown
Member

One question I have is about the apply method in TimestampsItemExt (and this question could be relevant to other extensions as well): if you pass null values for optional arguments defining properties that are already defined in that item, should the null value override the existing property?

That seems correct to me. I would expect that if None is passed in or if the param defaults to None, that would set the value accordingly

Copy link
Copy Markdown
Member

@lossyrob lossyrob left a comment

Choose a reason for hiding this comment

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

Looking good, comments inline.

Comment thread pystac/extensions/timestamps.py
Comment thread pystac/extensions/timestamps.py
Comment thread tests/extensions/test_timestamps.py
@lossyrob
Copy link
Copy Markdown
Member

Also note that you should add a CHANGELOG entry for this. Thanks!

@lossyrob
Copy link
Copy Markdown
Member

Should have mentioned this before, but can you add an api.rst doc entry for the extension?

@simonkassel
Copy link
Copy Markdown
Contributor Author

sure, will do

@lossyrob lossyrob merged commit bdf082c into develop Aug 20, 2020
@lossyrob lossyrob deleted the sjk/timestamps-extension branch August 20, 2020 23:58
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.

Implement extension: Timestamps

2 participants