Re: Review Request 2: Initial Blocker Review Code

[Martin Krizek]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard-tflink.rhcloud.com/r/2/#review6
-----------------------------------------------------------

blockerbugs/config.py
http://reviewboard-tflink.rhcloud.com/r/2/#comment5

Why is this commented out?

blockerbugs/controllers/main.py
http://reviewboard-tflink.rhcloud.com/r/2/#comment6

And why is this commented out? :)

blockerbugs/util/bz_interface.py
http://reviewboard-tflink.rhcloud.com/r/2/#comment7

The url could be loaded from the config, right?

blockerbugs/util/bz_interface.py
http://reviewboard-tflink.rhcloud.com/r/2/#comment8

Just curious, why is this not loaded in the constructor?

- Martin Krizek

On Feb. 7, 2013, 9:01 p.m., Tim Flink wrote:
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard-tflink.rhcloud.com/r/2/
>
> Review request for blockerbugs.
>
> Bugs: 334
>     https://fedorahosted.org/fedora-qa/ticket/334
>
> Repository: blockerbugs
> Description
>
> Initial code for blocker submission page
> Diffs
>
> testing/test_bugchange.py PRE-CREATION 
>   init_f18db.sh c20df66c4b4a8f38ab79a1b61043d26de112f71c 
>   init_db.sh PRE-CREATION 
>   blockerbugs/util/bz_interface.py 2bae79db2d3a1206b7de1cee5f005f137ba2d6f3 
>   blockerbugs/templates/thanks.html PRE-CREATION 
>   blockerbugs/templates/propose_bug.html PRE-CREATION 
>   blockerbugs/templates/base_nav.html f3a89798a9dadf297e722c067ad3795a72ebbf74 
>   blockerbugs/controllers/main.py e7525f8433d74802f7c0cb2d719f0abdc13431c4 
>   blockerbugs/controllers/forms.py 669d78a5030904d4559d6d68af093bd6f096888b 
>   blockerbugs/config.py 009e7a97f57e31fcc499dba87fe7d33e41252a2c 
>   blockerbugs/__init__.py ad051ca2523aa70ca2d4d26843e9a2bdf0b5cd00
>
> Diff: http://reviewboard-tflink.rhcloud.com/r/2/diff/
> Testing
>
> Thanks,
>
> Tim Flink

Review Request 2: Initial Blocker Review Code

[Martin Krizek]

> On Feb. 22, 2013, 9:24 a.m., Martin Krizek wrote:
> > blockerbugs/util/bz_interface.py, line 185
> > http://reviewboard-tflink.rhcloud.com/r/2/diff/1/?file=19#file19line185
> > Just curious, why is this not loaded in the constructor?

Part of it is a stylistic thing - I don't like constructors that load remote data. I've run into issues in the past with constructors that take too long or load a bunch of data wrt testability. It's not so much of an issue here since the loading can be overridden with a mock bz object but like I said, it's partially a stylistic thing on my part.

Do you think that the code would be easier to read or better if that was loaded in the constructor?
> On Feb. 22, 2013, 9:24 a.m., Martin Krizek wrote:
> > blockerbugs/util/bz_interface.py, line 48
> > http://reviewboard-tflink.rhcloud.com/r/2/diff/1/?file=19#file19line48
> > The url could be loaded from the config, right?

Yeah and it probably should. When the code was first written, the bugzilla URL wasn't in the app config and while normally, I'd say "let's not load the whole app just to get a config value", we're already doing that implicitly when we load the DB models so it's not a big issue.

- Tim

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard-tflink.rhcloud.com/r/2/#review6
-----------------------------------------------------------

On Feb. 22, 2013, 2:41 p.m., Tim Flink wrote:
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard-tflink.rhcloud.com/r/2/
>
> (Updated Feb. 22, 2013, 2:41 p.m.)
>
> Review request for blockerbugs.
>
> Bugs: 334
>     https://fedorahosted.org/fedora-qa/ticket/334
>
> Repository: blockerbugs
> Description
>
> Initial code for blocker submission page
> Diffs
>
> testing/test_bugchange.py PRE-CREATION 
>   init_f18db.sh c20df66c4b4a8f38ab79a1b61043d26de112f71c 
>   init_db.sh PRE-CREATION 
>   blockerbugs/util/bz_interface.py 2bae79db2d3a1206b7de1cee5f005f137ba2d6f3 
>   blockerbugs/templates/thanks.html PRE-CREATION 
>   blockerbugs/templates/propose_bug.html PRE-CREATION 
>   blockerbugs/templates/base_nav.html f3a89798a9dadf297e722c067ad3795a72ebbf74 
>   blockerbugs/controllers/main.py e7525f8433d74802f7c0cb2d719f0abdc13431c4 
>   blockerbugs/controllers/forms.py 669d78a5030904d4559d6d68af093bd6f096888b 
>   blockerbugs/config.py 009e7a97f57e31fcc499dba87fe7d33e41252a2c 
>   blockerbugs/__init__.py ad051ca2523aa70ca2d4d26843e9a2bdf0b5cd00
>
> Diff: http://reviewboard-tflink.rhcloud.com/r/2/diff/
> Testing
>
> Thanks,
>
> Tim Flink

Review Request 2: Initial Blocker Review Code

[Martin Krizek]

> On Feb. 22, 2013, 9:24 a.m., Martin Krizek wrote:
> > blockerbugs/util/bz_interface.py, line 185
> > http://reviewboard-tflink.rhcloud.com/r/2/diff/1/?file=19#file19line185
> > Just curious, why is this not loaded in the constructor?
>
> Tim Flink wrote:
>     Part of it is a stylistic thing - I don't like constructors that load remote data. I've run into issues in the past with constructors that take too long or load a bunch of data wrt testability. It's not so much of an issue here since the loading can be overridden with a mock bz object but like I said, it's partially a stylistic thing on my part.
> Do you think that the code would be easier to read or better if that was loaded in the constructor?

Not necessarily. I was just curious if there is another reason other than loading remote data.

- Martin

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard-tflink.rhcloud.com/r/2/#review6
-----------------------------------------------------------

On Feb. 22, 2013, 2:41 p.m., Tim Flink wrote:
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard-tflink.rhcloud.com/r/2/
>
> (Updated Feb. 22, 2013, 2:41 p.m.)
>
> Review request for blockerbugs.
>
> Bugs: 334
>     https://fedorahosted.org/fedora-qa/ticket/334
>
> Repository: blockerbugs
> Description
>
> Initial code for blocker submission page
> Diffs
>
> testing/test_bugchange.py PRE-CREATION 
>   init_f18db.sh c20df66c4b4a8f38ab79a1b61043d26de112f71c 
>   init_db.sh PRE-CREATION 
>   blockerbugs/util/bz_interface.py 2bae79db2d3a1206b7de1cee5f005f137ba2d6f3 
>   blockerbugs/templates/thanks.html PRE-CREATION 
>   blockerbugs/templates/propose_bug.html PRE-CREATION 
>   blockerbugs/templates/base_nav.html f3a89798a9dadf297e722c067ad3795a72ebbf74 
>   blockerbugs/controllers/main.py e7525f8433d74802f7c0cb2d719f0abdc13431c4 
>   blockerbugs/controllers/forms.py 669d78a5030904d4559d6d68af093bd6f096888b 
>   blockerbugs/config.py 009e7a97f57e31fcc499dba87fe7d33e41252a2c 
>   blockerbugs/__init__.py ad051ca2523aa70ca2d4d26843e9a2bdf0b5cd00
>
> Diff: http://reviewboard-tflink.rhcloud.com/r/2/diff/
> Testing
>
> Thanks,
>
> Tim Flink