[pinpoint] Refactor Quest Generator.

[pinpoint] Refactor Quest Generator.

Although it appeared more organized to separate the argument validation, Quest
generation, and property collection in the QuestGenerator object, it quickly
became more complex as more conditional logic was added, since that had to be
duplicated across the three concerns. Having a single linear code path leads to
less code duplication.

Review-Url: https://codereview.chromium.org/3002903002
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/6497b94a07fe6251a473ad1743a1444e7b0e1787

[dtu, 2017-08-17 22:31:44]

Description was changed from

==========
[pinpoint] Alternative Quest Generator.

Although it appeared more organized to separate the argument validation, Quest
generation, and property collection in the QuestGenerator object, it quickly
became more complex as more conditional logic was added, since that had to be
duplicated across the three concerns. Having a single linear code path leads to
less code duplication.
==========

to

==========
[pinpoint] Refactor Quest Generator.

Although it appeared more organized to separate the argument validation, Quest
generation, and property collection in the QuestGenerator object, it quickly
became more complex as more conditional logic was added, since that had to be
duplicated across the three concerns. Having a single linear code path leads to
less code duplication.
==========

[dtu, 2017-08-17 22:31:44]

dtu@chromium.org changed reviewers:
+ perezju@chromium.org, simonhatch@chromium.org, sullivan@chromium.org

[dtu, 2017-08-17 22:32:42]

Wdyt of this alternative to QuestGenerator?

[dtu, 2017-08-17 22:33:09]

On 2017/08/17 22:32:42, dtu wrote:
> Wdyt of this alternative to QuestGenerator?

I can add tests if we decide this is the better way to go.

[perezju, 2017-08-18 09:24:05]

On 2017/08/17 22:33:09, dtu wrote:
> Although it appeared more organized to separate the
> argument validation, Quest generation, and property
> collection in the QuestGenerator object, it quickly
> became more complex as more conditional logic was
> added, since that had to be duplicated across the
> three concerns. Having a single linear code path
> leads to less code duplication.

Which bits grew more complicated or had to be duplicated?

This looks to me as a step back in terms of code organization. Looking at your
implementation, though, it looks like what you really want is a function (may or
may not have to be a class) that takes as input the request object and returns a
pair of (arguments, quests). For example, how about replacing the
quest_generator.py module with something like your code in lines 85-160?

[dtu, 2017-08-21 23:26:39]

On 2017/08/18 09:24:05, perezju wrote:
> On 2017/08/17 22:33:09, dtu wrote:
> > Although it appeared more organized to separate the
> > argument validation, Quest generation, and property
> > collection in the QuestGenerator object, it quickly
> > became more complex as more conditional logic was
> > added, since that had to be duplicated across the
> > three concerns. Having a single linear code path
> > leads to less code duplication.
> 
> Which bits grew more complicated or had to be duplicated?
> 
> This looks to me as a step back in terms of code organization. Looking at your
> implementation, though, it looks like what you really want is a function (may
or
> may not have to be a class) that takes as input the request object and returns
a
> pair of (arguments, quests). For example, how about replacing the
> quest_generator.py module with something like your code in lines 85-160?

Sorry, not sure what you're suggesting.
Keep the new code, but move it into its own module?

Other option is to keep the QuestGenerator class, but include a wrapper function
to return (args, quests) and make the QuestGenerator classes private.

The problem is that the QuestGenerator code is not correct, and its structure
makes it hard to tell that it's not.
* AsDict() returns optional parameters, even if they're not set.
* Interactions of optional and required parameters aren't correct. E.g. if you
set `dimensions`, you're required to set `browser` as well. It's not obvious
we're supposed to validate that in the constructor, and it would be clearer if
we validate each parameter as it's used in Quests().

[perezju, 2017-08-22 09:43:33]

On 2017/08/21 23:26:39, dtu wrote:
> On 2017/08/18 09:24:05, perezju wrote:
> > On 2017/08/17 22:33:09, dtu wrote:
> > > Although it appeared more organized to separate the
> > > argument validation, Quest generation, and property
> > > collection in the QuestGenerator object, it quickly
> > > became more complex as more conditional logic was
> > > added, since that had to be duplicated across the
> > > three concerns. Having a single linear code path
> > > leads to less code duplication.
> > 
> > Which bits grew more complicated or had to be duplicated?
> > 
> > This looks to me as a step back in terms of code organization. Looking at
your
> > implementation, though, it looks like what you really want is a function
(may
> or
> > may not have to be a class) that takes as input the request object and
returns
> a
> > pair of (arguments, quests). For example, how about replacing the
> > quest_generator.py module with something like your code in lines 85-160?
> 
> Sorry, not sure what you're suggesting.
> Keep the new code, but move it into its own module?
> 
> Other option is to keep the QuestGenerator class, but include a wrapper
function
> to return (args, quests) and make the QuestGenerator classes private.
> 
> The problem is that the QuestGenerator code is not correct, and its structure
> makes it hard to tell that it's not.
> * AsDict() returns optional parameters, even if they're not set.
> * Interactions of optional and required parameters aren't correct. E.g. if you
> set `dimensions`, you're required to set `browser` as well. It's not obvious
> we're supposed to validate that in the constructor, and it would be clearer if
> we validate each parameter as it's used in Quests().

"Keep the new code, but move it into its own module" sgtm.

And yes, the interface should be just a function that takes a request object
and returns the (args, quests) pair.

[dtu, 2017-08-23 00:22:23]

Here it is, in its own module with tests.

Also considering creating a Quest.ParseArgs(), so that each individual Quest can
collect the args it needs.

[perezju, 2017-08-23 08:59:59]

lgtm w/few nits and comments

> Also considering creating a Quest.ParseArgs(), so that each 
> individual Quest can collect the args it needs.

That may also be a good idea. But lets land this for now and consider that as a
follow up.

https://codereview.chromium.org/3002903002/diff/20001/dashboard/dashboard/pin...
File dashboard/dashboard/pinpoint/handlers/new_test.py (right):

https://codereview.chromium.org/3002903002/diff/20001/dashboard/dashboard/pin...
dashboard/dashboard/pinpoint/handlers/new_test.py:162: pass
missing something here?

https://codereview.chromium.org/3002903002/diff/20001/dashboard/dashboard/pin...
File dashboard/dashboard/pinpoint/handlers/quest_parser.py (right):

https://codereview.chromium.org/3002903002/diff/20001/dashboard/dashboard/pin...
dashboard/dashboard/pinpoint/handlers/quest_parser.py:1: # Copyright 2017 The
Chromium Authors. All rights reserved.
nit: nit: I don't know if "parser" is the right word here (I think as a parser
as something that takes as input a block of text). Maybe "quest_maker",
"quest_factory"? "quest_generator" was actually fine too.

https://codereview.chromium.org/3002903002/diff/20001/dashboard/dashboard/pin...
dashboard/dashboard/pinpoint/handlers/quest_parser.py:19: def
ParseQuests(request):
Add some docstring explaining the args it takes and return value.

https://codereview.chromium.org/3002903002/diff/20001/dashboard/dashboard/pin...
dashboard/dashboard/pinpoint/handlers/quest_parser.py:29: return arguments,
quests
nit: I think it's clearer the other way around, so you have positive boolean
tests and a single exit point at the end.

if dimensions:
  # stuff to do if there are dimensions

return arguments, quests

https://codereview.chromium.org/3002903002/diff/20001/dashboard/dashboard/pin...
dashboard/dashboard/pinpoint/handlers/quest_parser.py:40: if not metric:
nit: Ditto.

https://codereview.chromium.org/3002903002/diff/20001/dashboard/dashboard/pin...
File dashboard/dashboard/pinpoint/handlers/quest_parser_test.py (right):

https://codereview.chromium.org/3002903002/diff/20001/dashboard/dashboard/pin...
dashboard/dashboard/pinpoint/handlers/quest_parser_test.py:48: expected = [
nit: expected_quests

https://codereview.chromium.org/3002903002/diff/20001/dashboard/dashboard/pin...
dashboard/dashboard/pinpoint/handlers/quest_parser_test.py:146:
self.assertEqual(quest_parser.ParseQuests(arguments), (arguments, expected))
hmm.. so the arguments we send as input are also normally the same as the
arguments we get as output? why do we need those? just for bookkeeping in the
Job object?

would it make sense to test cases where the output arguments are not identical
to the input ones?

[perezju, 2017-08-23 09:16:11]

https://codereview.chromium.org/3002903002/diff/20001/dashboard/dashboard/pin...
File dashboard/dashboard/pinpoint/handlers/quest_parser.py (right):

https://codereview.chromium.org/3002903002/diff/20001/dashboard/dashboard/pin...
dashboard/dashboard/pinpoint/handlers/quest_parser.py:29: return arguments,
quests
On 2017/08/23 08:59:59, perezju wrote:
> nit: I think it's clearer the other way around, so you have positive boolean
> tests and a single exit point at the end.
> 
> if dimensions:
>   # stuff to do if there are dimensions
> 
> return arguments, quests

I've just seen the other patch that goes on top of this. The way that works out
looks very clear. You can ignore this comment here.

[dtu, 2017-08-24 23:17:52]

https://codereview.chromium.org/3002903002/diff/20001/dashboard/dashboard/pin...
File dashboard/dashboard/pinpoint/handlers/new_test.py (right):

https://codereview.chromium.org/3002903002/diff/20001/dashboard/dashboard/pin...
dashboard/dashboard/pinpoint/handlers/new_test.py:162: pass
On 2017/08/23 08:59:58, perezju (OOO back on 30 Aug) wrote:
> missing something here?

Done. Removed. These tests are now under quest_generator_test.py.

https://codereview.chromium.org/3002903002/diff/20001/dashboard/dashboard/pin...
File dashboard/dashboard/pinpoint/handlers/quest_parser.py (right):

https://codereview.chromium.org/3002903002/diff/20001/dashboard/dashboard/pin...
dashboard/dashboard/pinpoint/handlers/quest_parser.py:1: # Copyright 2017 The
Chromium Authors. All rights reserved.
On 2017/08/23 08:59:58, perezju (OOO back on 30 Aug) wrote:
> nit: nit: I don't know if "parser" is the right word here (I think as a parser
> as something that takes as input a block of text). Maybe "quest_maker",
> "quest_factory"? "quest_generator" was actually fine too.

Done.

https://codereview.chromium.org/3002903002/diff/20001/dashboard/dashboard/pin...
dashboard/dashboard/pinpoint/handlers/quest_parser.py:19: def
ParseQuests(request):
On 2017/08/23 08:59:59, perezju (OOO back on 30 Aug) wrote:
> Add some docstring explaining the args it takes and return value.

Done.

https://codereview.chromium.org/3002903002/diff/20001/dashboard/dashboard/pin...
dashboard/dashboard/pinpoint/handlers/quest_parser.py:29: return arguments,
quests
On 2017/08/23 09:16:11, perezju (OOO back on 30 Aug) wrote:
> On 2017/08/23 08:59:59, perezju wrote:
> > nit: I think it's clearer the other way around, so you have positive boolean
> > tests and a single exit point at the end.
> > 
> > if dimensions:
> >   # stuff to do if there are dimensions
> > 
> > return arguments, quests
> 
> I've just seen the other patch that goes on top of this. The way that works
out
> looks very clear. You can ignore this comment here.

Acknowledged.

https://codereview.chromium.org/3002903002/diff/20001/dashboard/dashboard/pin...
dashboard/dashboard/pinpoint/handlers/quest_parser.py:40: if not metric:
On 2017/08/23 08:59:58, perezju (OOO back on 30 Aug) wrote:
> nit: Ditto.

Acknowledged.

https://codereview.chromium.org/3002903002/diff/20001/dashboard/dashboard/pin...
File dashboard/dashboard/pinpoint/handlers/quest_parser_test.py (right):

https://codereview.chromium.org/3002903002/diff/20001/dashboard/dashboard/pin...
dashboard/dashboard/pinpoint/handlers/quest_parser_test.py:48: expected = [
On 2017/08/23 08:59:59, perezju (OOO back on 30 Aug) wrote:
> nit: expected_quests

Done.

https://codereview.chromium.org/3002903002/diff/20001/dashboard/dashboard/pin...
dashboard/dashboard/pinpoint/handlers/quest_parser_test.py:146:
self.assertEqual(quest_parser.ParseQuests(arguments), (arguments, expected))
On 2017/08/23 08:59:59, perezju (OOO back on 30 Aug) wrote:
> hmm.. so the arguments we send as input are also normally the same as the
> arguments we get as output? why do we need those? just for bookkeeping in the
> Job object?
> 
> would it make sense to test cases where the output arguments are not identical
> to the input ones?

We mostly just want them to display the configuration cleanly in the UI,
separately from the change list and other request stuff.
Not sure if there's a more automated way to do it.

Originally we were going to have some that weren't the same as the input, but it
seems like that's never the case here:
* repeat_count was supposed to be an int, but since we're just passing it
through to swarming as a string, we don't need to do the conversion.
* dimensions is a dict, but it's not really important if we show the original
argument as given or the re-JSONified version.

[dtu, 2017-08-24 23:17:57]

The CQ bit was checked by dtu@chromium.org

[dtu, 2017-08-24 23:17:57]

The patchset sent to the CQ was uploaded after l-g-t-m from perezju@chromium.org
Link to the patchset: https://codereview.chromium.org/3002903002/#ps40001
(title: "Comments")

[commit-bot: I haz the power, 2017-08-24 23:18:06]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-08-24 23:44:58]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1503616677346370,
"parent_rev": "6f510b70e615fe4d5d536f66015adba45f8e43c2", "commit_rev":
"6497b94a07fe6251a473ad1743a1444e7b0e1787"}

[commit-bot: I haz the power, 2017-08-24 23:45:01]

Description was changed from

==========
[pinpoint] Refactor Quest Generator.

Although it appeared more organized to separate the argument validation, Quest
generation, and property collection in the QuestGenerator object, it quickly
became more complex as more conditional logic was added, since that had to be
duplicated across the three concerns. Having a single linear code path leads to
less code duplication.
==========

to

==========
[pinpoint] Refactor Quest Generator.

Although it appeared more organized to separate the argument validation, Quest
generation, and property collection in the QuestGenerator object, it quickly
became more complex as more conditional logic was added, since that had to be
duplicated across the three concerns. Having a single linear code path leads to
less code duplication.

Review-Url: https://codereview.chromium.org/3002903002
Committed:
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
==========

[commit-bot: I haz the power, 2017-08-24 23:45:03]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...