Nobackpans

[mavorg]

This is my take to hide "no latest" distro from favorites. See: metacpan/metacpan-api#397.
The approach is to give a reusable method for any future re-use.
Lacks related testing, working on it...

[mavorg]

http://irclog.perlgeek.de/metacpan/2015-10-29#i_11452331 @oalders:
I actually started the patch in that direction (by filtering out all the no-latest) but then went the other way because:

1. Reading some of the code, the model is just a thin layer over the ES api and all the methods seems to do just one thing and do it well assuming nothing of what the user want to do with data: as I see it, that method is very low-level and could be re-used in other areas where "no-latest" are unwanted or "wanted".
2. It seems to me that "num. of latest distros" >> "num. of not latest distros" (at least this is true in favorites), so memory-wise the returned data structure is always small.
3. I preferred hashes over arrays because of access speed: consider that by returning an array of "latest distros" will end up in an O(n^2) filtering in the controller. keys %hash could even make happy someone which want an array.

Either way (returning latest or the no-latest) you still need some filter code to filter them in the controller, so it is maybe a question of preference.

[oalders]

I don't feel too strongly about latest vs no latest. I'm ok with the model making some assumptions about what the user wants, but I see where you're coming from.

The hash access will be slightly faster, so that's fine, but why not return a hash keyed on the distribution name? I'm not sure what we win by checking $noLatest->{no_latest}->{$distro} rather than $no_latest->{$distro}

[oalders]

Could we call this variable $no_latest? We've been avoiding snake case in general.

[mavorg]

The hash access will be slightly faster, so that's fine, but why not return a hash keyed on the distribution name? I'm not sure what we win by checking $noLatest->{no_latest}->{$distro} rather than $no_latest->{$distro}

I don't know if it matters or not but I tried to mimic ES data structures (seen that all the other methods returns an hash with some metadata and results are nested under the "hits" key) because I thought that metadata is still important (see the took key):

{
    took      => $data->{took},
    no_latest => {
        map {
            my $distro = $_;
            ( first { $_ eq $distro } @latest )
            ? ()
            : ( $distro, 1 );
        } @distributions
    }
}

As bonus you can easily check if there are any hits at all by inspecting the no_latest hash's key number.
Maybe it could be renamed to "hits", so then you can access it with $noLatest->{hits}->{$distro} which is semantically better.

[oalders]

I get why you did it -- it makes sense to me, but we don't really need to mimic the ES structure in this case (or worry about how long the query took). The purpose of the model is to take the ES data structures and massage them into something we can easily work with.

In this case the it's basically a question of whether or not a condition is true. So for that, the structure above is a lot more complex than we need.

It's a big codebase with a lot of code to follow, so my inclination is to merge only what we need moving forward. If we need more info at some point in the future, we can rework the model, but we're best off avoiding all increased complexity if we don't actually need it right now. In this case returning an array of dist names would be my preference. We can always use a map to create a hash if we need it. Or we can return the hash, but the extra keys aren't adding value just yet.

[mavorg]

@oalders I need guidance with the testing part for this patch: should I just query the issue's OP author name and check that the "backpans" are flagged appropriately or should I mock an ES result and pass it to the no_latest method?
Should I also check that the html does not contain backpans?

[oalders]

@margeas good question. I don't think we have a good way of mocking at this point. A visual check of the author page in question would be good. Also, we can set the test up to query the live API. We can test that nothing explodes and that something useful is returned. There are other tests which query the live API, so that's fine. That would be the 80% solution. The API data will always be in flux, so the test should account for that (ie don't expect to get something very specific back).

[oalders]

@margeas is this still on your TODO list? Just wondering what the status is. It will need a rebase at the very least.

[mavorg]

On Mon, 11 Jan 2016, Olaf Alders wrote:

@margeas is this still on your TODO list? Just wondering what the status is. It will need a rebase at the very least.

Hi Olaf, yes I've been working on and off to integrate your suggested
changes to the patch and it lacks only the testing part.

Unfortunately in the last few weeks I had major problems with my main
workstation machine and I have all my tasks shifted.
My plan is to be back on track working on it the next week.

---

Reply to this email directly or view it on GitHub:
#1609 (comment)

[oalders]

@margeas I think this will need a rebase. Would be good if we could merge this in by the end of March. Do you think that's possible?

[mavorg]

On 03/14/2016 04:04 AM, Olaf Alders wrote:

@margeas https://github.com/margeas I think this will need a rebase.
Would be good if we could merge this in by the end of March. Do you
think that's possible?

—
Reply to this email directly or view it on GitHub
#1609 (comment).

Hi Olaf, I should be able to rebase it with the needed mods before the
end of month, but without the testing part.

[oalders]

That sounds good. Please ping me after the rebase so I don't lose track of this.

[oalders]

Closing as there is now a rebased version at #1662