lint: result gathering pattern smells

[Robie Basak]

From: https://code.launchpad.net/~nacc/usd-importer/+git/usd-importer/+merge/327399

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

[Robie Basak (racb)]

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.

[Nish Aravamudan (nacc)]

Thanks for this suggestion, I'd not seen that pattern before, and I like
the idea of two internal objects with truthiness and print-ability.

On Jul 25, 2017 11:12, "Robie Basak" <email address hidden> wrote:

> 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.
>
> --
> You received this bug notification because you are a member of Ubuntu
> Server Dev import team, which is subscribed to usd-importer.
> https://bugs.launchpad.net/bugs/1706413
>
> Title:
> lint: result gathering pattern smells
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/usd-importer/+bug/1706413/+subscriptions
>