Skip to content

Conversation

LeonLu2
Copy link
Collaborator

@LeonLu2 LeonLu2 commented Jan 17, 2023

closes #672

Prerequisites:

  • Unless it is a documentation hotfix it should be merged against the dev branch
  • Branch is up-to-date with the branch to be merged with, i.e. dev
  • Build is successful
  • Code is cleaned up and formatted

Summary

  1. add the placeholder option for time_values and issues in queries
  2. add unittests of time_values=* and issues=*

@LeonLu2 LeonLu2 requested a review from krivard January 17, 2023 05:20
krivard
krivard previously approved these changes Jan 19, 2023
Copy link
Contributor

@krivard krivard left a comment

Choose a reason for hiding this comment

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

Looks good! Small adjustments.

LeonLu2 and others added 2 commits January 19, 2023 12:28
Co-authored-by: Katie Mazaitis <krivard@cs.cmu.edu>
Co-authored-by: Katie Mazaitis <krivard@cs.cmu.edu>
Copy link
Contributor

@krivard krivard left a comment

Choose a reason for hiding this comment

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

👍

@krivard krivard merged commit 04ed491 into dev Jan 19, 2023
@krivard krivard deleted the leonlu2/time_values_and_issues branch January 19, 2023 18:41
Copy link
Collaborator

@melange396 melange396 left a comment

Choose a reason for hiding this comment

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

its a little late now since this was already merged, but i had a couple style points...

Comment on lines 383 to +387
try:
return int(s.replace("-", ""))
if s == "*":
return s
else:
return int(s.replace("-", ""))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
try:
return int(s.replace("-", ""))
if s == "*":
return s
else:
return int(s.replace("-", ""))
if s == "*":
return s
try:
return int(s.replace("-", ""))

the if "*" doesnt need to be in the try block (which is there to make sure s is made up of numbers (and dashes)), and the else is superfluous since the return happens right above it.

Comment on lines +477 to +481
if issues == ["*"]:
self.retable(history_table)
else:
self.retable(history_table)
self.where_integers("issue", issues)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if issues == ["*"]:
self.retable(history_table)
else:
self.retable(history_table)
self.where_integers("issue", issues)
self.retable(history_table)
if issues != ["*"]:
self.where_integers("issue", issues)

you dont have to repeat the retable() line, and the where_integers() call is better done under the if instead of the else (by reversing the comparison).

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.

covidcast: support issues=*, time_values=* queries

3 participants