Updated unique terms to work with previous functionality, and fixed a pagination bug#221
Conversation
| self.total_row_count = next_result.total_row_count | ||
| return next_result | ||
| return None | ||
| # self.total_row_count = next_result.total_row_count |
There was a problem hiding this comment.
Please remove commented out code
otchet-broad
left a comment
otchet-broad
left a comment
There was a problem hiding this comment.
Thanks for the PR.
I left a few comments for some things that need cleanup, but overall seems good. I'm glad you're seeing improvements and identifying issues by adding test coverage.
ahaessly
left a comment
ahaessly
left a comment
There was a problem hiding this comment.
a couple of minor comments
| from cdapython.results.columns_result import ColumnsResult | ||
| from cdapython.results.factories.collect_result import CollectResult | ||
| from cdapython.results.page_result import get_query_result | ||
| from cdapython.results.page_result import Paged_Result, get_query_result |
There was a problem hiding this comment.
a nit pick - usually I would expect to see either PagedResult or paged_result. This seems like a strange mix of the two. was there a specific reason to do this?
There was a problem hiding this comment.
I just messed up with the naming convention when naming the class Paged_Result. https://peps.python.org/pep-0008/#descriptive-naming-styles
|
|
||
| def test_unique_terms() -> None: | ||
| terms = unique_terms("sex", "GDC").run(host=host, show_term_count=True) | ||
| terms = unique_terms(col_name="sex", system="GDC", host=host, show_term_count=True) |
There was a problem hiding this comment.
does this require a change to the documentation? If so, I think we should add a ticket for Amanda so she knows what to update in the documentation.
There was a problem hiding this comment.
This brings the functionality back in line with the documentation.
otchet-broad
left a comment
otchet-broad
left a comment
There was a problem hiding this comment.
Thanks @dionboles-asym . Please note in the ticket for CD-646 that a new branch and pull request will need to be created to complete the work.
3 participants
This branch I updated unique terms to reference previous functionality and I fixed a pagination bug. I also had to remove the run method from all of my test.