-
Notifications
You must be signed in to change notification settings - Fork 49
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
feat(projects): add check for finding next relevant page #1192
base: 2.3.0
Are you sure you want to change the base?
feat(projects): add check for finding next relevant page #1192
Conversation
9796e9e
to
f3b8850
Compare
f3b8850
to
9756d2c
Compare
3e06cd8
to
c90ea63
Compare
c90ea63
to
f788af7
Compare
Signed-off-by: David Wallace <[email protected]>
f4d75ac
to
6f8244e
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.
Awesome, works nicely. Do we test this? Otherwise only style feedback.
rdmo/projects/progress.py
Outdated
max_iterations = len(catalog.pages) | ||
iterations = 0 | ||
|
||
while iterations < max_iterations: |
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 could be a for loop over catalog pages (which should be ordered) beginning with current_page
. Maybe this would be more elegant, idk, you decide.
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.
Or a recursive function 😈
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.
ah yes, a recursive function also goes there, it makes the function very compact and simple actually
values = self.project.values.filter(snapshot=None).select_related('attribute', 'option') | ||
|
||
if check_conditions(conditions, values): | ||
sets = compute_sets(values) |
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 wonder how much more expensive this is. In the future we could move functions like compute_sets
to the manager. Just having ideas...
ok thanks! Yes think that a certain specific test case would be maybe appropriate here, I'll add one. |
Signed-off-by: David Wallace <[email protected]>
Description
Related issue: #1191
Adds the method
compute_next_relevant_page
inprogress.py
.Refactors
compute_show_page
to be able to re-used inProjectPageViewSet
for finding the next relevant page.Motivation and Context
How has this been tested?
Screenshots (if appropriate)