-
-
Notifications
You must be signed in to change notification settings - Fork 316
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
Minor refactor recycled from closed PR #1174
Conversation
…thesar into distinct_only_preprocessed
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 pretty good overall. I think we should move the column id to name wrangling out of the viewset, and down into a model utility, since it's more related to the models (and so that would be easier to find/understand later).
if limit: | ||
selectable = selectable.limit(limit) |
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.
Since you've set things up with default None
s in the function sig, this conditional isn't needed: limit(self, limit)
is a no-op when limit = None
.
if offset: | ||
selectable = selectable.offset(offset) |
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.
Same as the limit. the conditional isn't needed.
selectable = selectable.cte() | ||
selectable = select(count_column).select_from(selectable) | ||
return execute_query(engine, selectable)[0][col_name] |
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.
Nice. More idiomatic of our code base.
mathesar/api/db/viewsets/records.py
Outdated
filter_processed = _rewrite_db_function_spec( | ||
filter_unprocessed, column_ids_to_names, | ||
) |
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 think this logic would fit much better in the models utility module, mathesar.utils.models
.
@mathemancer in reply to: > I think this logic would fit much better in the models utility module, The actual logic that does the rewriting is actually in the I removed this method, unwrapping the call to the rewriting method. I think it was better before. If you agree, we can revert back. @mathemancer in reply to: I'm aware that some of the Can we keep the redundant |
Codecov Report
@@ Coverage Diff @@
## master #1174 +/- ##
==========================================
- Coverage 93.39% 93.32% -0.08%
==========================================
Files 113 113
Lines 4332 4360 +28
==========================================
+ Hits 4046 4069 +23
- Misses 286 291 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
My two cents is that it's more confusing to have a redundant I don't think we should be using redundant code or code structure generally to convey ideas (like a pipish model or a need for refactoring), everyone reads code differently. Comments would be more explicit and less confusing. |
@kgodey in this particular case, it's more confusing to have something not Some comments might be a good idea, but some code structure can be worth a lot of comments. |
@dmos62 I'm skeptical that the code structure will send all (or even most) readers only that message given the larger context of the function with default However, I'll defer to @mathemancer's judgment here on what to do with the code since he's the reviewer. |
I strongly prefer the separate function version to inline. My problem is having non-exception branching in a viewset at all. That said, as I look at our other viewset code, maybe I'm in the minority there. I'm okay with reverting to the wrapped version, getting that in, and reassessing where we want what logic later.
I think this will be a problem, since different pipeline bits might treat |
This pull request has not been updated in 45 days and is being marked as stale. It will automatically be closed in 30 days if not updated by then. |
This PR will likely be closed, since it's being superseded by #1222, but I'm keeping it open until after I merge the other one, since they might not completely overlap. |
This is a small collection of refactors recycled from a rejected PR (#1148).
It's also a minor extension of the filters endpoint to show some information relevant for equality-based filters. This will become useful again when we come back to the issue addressed by #1148. No breaking changes.
Some changes are only formatting, because, for example, I added some parameters to a function in the PR this is based on, making the declaration multi-line, but then removed the new parameters for this PR, after which only formatting changes were left. I don't think any of the formatting changes are detrimental on their own, so I didn't bother undoing them.
Checklist
Update index.md
).master
branch of the repositoryvisible errors.
Developer Certificate of Origin
Developer Certificate of Origin