-
-
Notifications
You must be signed in to change notification settings - Fork 320
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
Record grouping #315
Record grouping #315
Conversation
Question before I open this for review - do we have a set place to handle errors for the functions were exposing in the API? As of now we don't have much explicit error handling except for some of the stuff that's automatically included in In practice, I think the most principled place for validation would be in the form object that we're using to parse the initial url parameters (This would also remove the awkward error handling in the middle of the |
@eito-fis I don't think I have enough information to make an informed recommendation. Questions:
|
Having written this out, re-implementing the validation doesn't seem like a great idea... |
@eito-fis Sounds like you've answered your own question, that all makes sense to me. |
This should be reviewed after #300 is merged. |
9fe6036
to
0df171e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the overall approach, and agree that pushing the validation down to the DB layer makes sense. Unfortunately, there's a bug occurring when grouping by a single column. See the comment in the pagination.py
file.
filters=filters, order_by=order_by | ||
) | ||
# Convert the tuple keys into strings so it can be converted to JSON | ||
group_count = {','.join(k): v for k, v in group_count.items()} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bug if a string rather than a tuple is returned by table.get_group_counts
, i.e., if we want to group by only one column. I suggest modifying that function so it only has one return type(ish), a tuple. This will be easier to deal with, and less prone to causing bugs. There should also be a test for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In particular, this results in splitting the value of that column up into its constituent characters in the output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thank you!
Fixes #216
Adds the ability to get group count information from the record list endpoint.
Technical details
Adds a
get_group_counts()
function to thedb
module, which takes in a list of fields to group by and returns a dictionary mapping tuples of fields to their counts. Also adds agroup_count_by
parameter to the record list endpoint, where a user can pass a list of fields to group by and getincluding in the return data. The keys for
results
are strings, with composite fields names joined by,
s.Checklist
Update index.md
).master
branch of the repositoryvisible errors.
Developer Certificate of Origin
Developer Certificate of Origin