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

Let date be a date, not just datetime; also default _at() methods to now() #235

Merged
merged 9 commits into from Mar 4, 2021

Conversation

jvavrek
Copy link
Contributor

@jvavrek jvavrek commented Feb 18, 2021

Closes #226

@jvavrek jvavrek changed the title let date be a date, not just datetime; also default _at() methods to now() Draft: Let date be a date, not just datetime; also default _at() methods to now() Feb 18, 2021
@jvavrek jvavrek requested a review from cosama February 18, 2021 23:18
@cosama
Copy link
Contributor

cosama commented Feb 18, 2021

Looks good to me. Might make sense to add a test for the datetime.date thing.

Also there is this test: https://github.com/lbl-anp/becquerel/blob/update-date/tests/isotope_qty_test.py#L249. We could add no argument to it.

Looking at that I realized that there is a *_now method as well. Maybe can deprecate that and simplify the interface by defaulting to now with the *_at methods. @markbandstra Any thoughts?

@jvavrek
Copy link
Contributor Author

jvavrek commented Mar 2, 2021

@cosama this should be good to go:

  • added a test for the datetime parse
  • added no argument to test_isotopequantity_activity_now()
  • deprecated the _now() methods

@jvavrek jvavrek changed the title Draft: Let date be a date, not just datetime; also default _at() methods to now() Let date be a date, not just datetime; also default _at() methods to now() Mar 2, 2021
tests/isotope_qty_test.py Outdated Show resolved Hide resolved
@cosama
Copy link
Contributor

cosama commented Mar 4, 2021

just realize there is only ref_atoms but no ref_bq or other units. Probably worth adding that? Here or new one? Maybe a ref_quantity as well?

Copy link
Member

@markbandstra markbandstra left a comment

Choose a reason for hiding this comment

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

Thanks!

@jvavrek
Copy link
Contributor Author

jvavrek commented Mar 4, 2021

just realize there is only ref_atoms but no ref_bq or other units. Probably worth adding that? Here or new one? Maybe a ref_quantity as well?

The ref_atoms docstring says:

Access the reference atoms directly (for backwards compatibility)

so I assume that self._ref_quantities is the preferred way.

@jvavrek jvavrek merged commit ac89d9a into main Mar 4, 2021
@jvavrek jvavrek deleted the update-date branch March 4, 2021 22:52
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.

IsotopeQuantity has a kwarg date that cannot be a datetime.date object
3 participants