lint: result gathering pattern smells
Affects | Status | Importance | Assigned to | Milestone | |
---|---|---|---|---|---|
git-ubuntu |
Triaged
|
Medium
|
Unassigned |
Bug Description
From: https:/
I'm not keen on this pattern. It feels error prone. How about yielding each False or True instead, and then using all() on the a call to that generator?
> It is, probably :) Not yet fixed, but I think I'd prefer to wrap the
> all() usage in the API? That is, I think this is equally confusing:
>
> ret = all(self.
>
> I was thinking we could drop the _check naming and make it check...
> and move the actual function into _check in this case?
I'm not sure I follow exactly. We should chat more on how we want this
to work I think. I suggest we leave it for now and try to improve just
this in separate MPs just for this, as there's no obviously right
pattern to use but we need to pick one.
Let's use this bug to discuss.
tags: | added: lint |
Changed in usd-importer: | |
milestone: | none → 1.0 |
status: | New → Triaged |
importance: | Undecided → Medium |
I think that if we treat each checking function as returning a sequence of booleans, telling us whether each check was successful or not, then yielding works. You can interpret the results of any check function using all(), any() or examining the individual booleans. A check function can use "yield from" to incorporate other check functions, and each check function can adjust its control flow according to previous checks (for example if a check isn't possible because a previous check failed).
I prefer this because I think that using "ret" on every line is particularly error-prone. It's repetitive code, which I think is the code smell here.
I admit that perhaps it's not great for the returned booleans not to be aligned with anything. We could further enhance the results by making them class instances that define __bool__ instead, but also carry more information about what failed. This would work quite nicely to separate stdout from the running of the checks for better testability. yield and all() would still work. We'd do something like:
yield LintError("X is not Y")
or:
yield LintSuccess("A is correct")
LintSuccess. __bool_ _() would always return True, and LintError. __bool_ _() would always return False.