Minor refactor recycled from closed PR

[dmos62]

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

• My pull request has a descriptive title (not a vague title like Update index.md).
• My pull request targets the master branch of the repository
• My commit messages follow best practices.
• My code follows the established code style of the repository.
• I added tests for the changes I made (if applicable).
• I added or updated documentation (if applicable).
• I tried running the project locally and verified that there are no
  visible errors.

Developer Certificate of Origin

Developer Certificate of Origin

Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

[mathemancer]

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).

[mathemancer]

Since you've set things up with default Nones in the function sig, this conditional isn't needed: limit(self, limit) is a no-op when limit = None.

[mathemancer]

Same as the limit. the conditional isn't needed.

[mathemancer]

Nice. More idiomatic of our code base.

[mathemancer]

I think this logic would fit much better in the models utility module, mathesar.utils.models.

[dmos62]

@mathemancer in reply to:

> I think this logic would fit much better in the models utility module, mathesar.utils.models.

The actual logic that does the rewriting is actually in the mathesar.functions.operations.convert namespace. This method in the records viewset file is just a wrapped call to that, which also adds handling for input being None, and includes a (hopefully helpful) doc string and some comments, so I didn't move it to another namespace.

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:

> Since you've set things up with default Nones in the function sig, this conditional isn't needed: limit(self, limit) is a no-op when limit = None.

I'm aware that some of the None checks are redundant. But, the idea here is to treat each transformation in this pipeline the same. I tried to underline their conceptual equality by wrapping each of them in an identical if block, even if it's redundant in a few cases. I wanted to make it obvious that this is ripe for some refactoring: maybe something that would make the pipish nature of this routine more explicit.

Can we keep the redundant if checks?

[codecov-commenter]

Codecov Report

Merging #1174 (4b82f5a) into master (08f5c83) will decrease coverage by 0.07%.
The diff coverage is 92.30%.

@@            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     

Flag	Coverage Δ
pytest-backend	93.32% <92.30%> (-0.08%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
db/functions/operations/apply.py	86.11% <0.00%> (-10.95%)	⬇️
db/types/email.py	100.00% <ø> (ø)
db/types/uri.py	100.00% <ø> (ø)
mathesar/models.py	95.88% <80.00%> (-0.37%)	⬇️
db/functions/base.py	93.29% <85.71%> (-0.34%)	⬇️
db/functions/hints.py	96.66% <100.00%> (+1.92%)	⬆️
db/functions/known_db_functions.py	95.23% <100.00%> (+1.90%)	⬆️
db/functions/operations/deserialize.py	92.30% <100.00%> (-1.25%)	⬇️
db/records/operations/select.py	100.00% <100.00%> (ø)
mathesar/api/db/viewsets/records.py	100.00% <100.00%> (ø)
... and 6 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 08f5c83...4b82f5a. Read the comment docs.

[kgodey]

I'm aware that some of the None checks are redundant. But, the idea here is to treat each transformation in this pipeline the same. I tried to underline their conceptual equality by wrapping each of them in an identical if block, even if it's redundant in a few cases. I wanted to make it obvious that this is ripe for some refactoring: maybe something that would make the pipish nature of this routine more explicit.

My two cents is that it's more confusing to have a redundant if check, if I read that code, I'd probably go down a rabbit hole trying to figure out why there's a conditional there when it seems unnecessary.

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.

[dmos62]

@kgodey in this particular case, it's more confusing to have something not None-checked. I think a reader would take a second to realise that the SA call probably does a no-op if it's passed a None, but he can't be sure, because the enclosing method might never get called with that parameter set to None. These "redundant" if blocks tell the reader "there's nothing to think about: sometimes this is None, and in that case this block doesn't happen".

Some comments might be a good idea, but some code structure can be worth a lot of comments.

[kgodey]

These "redundant" if blocks tell the reader "there's nothing to think about: sometimes this is None, and in that case this block doesn't happen".

@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 None arguments.

However, I'll defer to @mathemancer's judgment here on what to do with the code since he's the reviewer.

[mathemancer]

@mathemancer in reply to:

> I think this logic would fit much better in the models utility module, mathesar.utils.models.

The actual logic that does the rewriting is actually in the mathesar.functions.operations.convert namespace. This method in the records viewset file is just a wrapped call to that, which also adds handling for input being None, and includes a (hopefully helpful) doc string and some comments, so I didn't move it to another namespace.

I removed this method, unwrapping the call to the rewriting method. I think it was better before. If you agree, we can revert back.

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.

@mathemancer in reply to:

> Since you've set things up with default Nones in the function sig, this conditional isn't needed: limit(self, limit) is a no-op when limit = None.

I'm aware that some of the None checks are redundant. But, the idea here is to treat each transformation in this pipeline the same. I tried to underline their conceptual equality by wrapping each of them in an identical if block, even if it's redundant in a few cases. I wanted to make it obvious that this is ripe for some refactoring: maybe something that would make the pipish nature of this routine more explicit.

Can we keep the redundant if checks?

I think this will be a problem, since different pipeline bits might treat None differently. There might be cases where some explicit None is used. Further, even in the case we have, this added complexity adds the potential for bugs, and these are more complicated to test for in this setting than they would be function-by-function. limit and offset treat the falsy 0 value differently in comparison to the falsy None. A query with a limit of 0 can be used to return just column names (here, that possibility is short-circuited), whereas the offset of 0 is equal to an offset of None (with our current logic). While this specific example can be sorted by changing all checks to if foo is None: rather than if foo:, my broader point is that what to do about a None input should belong to the bit of the pipeline rather than to the caller, and tested for each part separately. In the cases of functions in the pipeline that don't do the right thing when handed None, I think we should instead get those sorted out bit by bit.

[github-actions]

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.

[dmos62]

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.