aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "John Sirois" <john.sir...@gmail.com>
Subject Re: Review Request 40310: Replace Twitter checkstyle with pants checkstyle.
Date Mon, 16 Nov 2015 17:29:46 GMT


> On Nov. 16, 2015, 9:27 a.m., Joshua Cohen wrote:
> > src/test/python/apache/aurora/admin/test_admin.py, lines 158-160
> > <https://reviews.apache.org/r/40310/diff/2/?file=1125865#file1125865line158>
> >
> >     Why kill this? It seems like the intention was to ensure that set quota is called
with the correct types, but the assert above still passes if we were to do `assert_called_with(role,
24, 4001.0, 6001.0)`, thus the explicit type checks were necessary?

Ah right - I was thinking this was a tautology but its not, `24.0 == 24` is `True`.
This action on my part was triggered by:
```
10:08:08 00:00   [compile]
10:08:08 00:00     [python-eval]
10:08:08 00:00     [pythonstyle]
                   Invalidated 9 targets.
E721:ERROR   src/test/python/apache/aurora/admin/test_admin.py:158 do not compare types, use
'isinstance()'
     |      assert type(api.set_quota.call_args[0][1]) == type(float())
```

I've switched to isinstance which also checks what you want; ie: `isinstance(24.0, int)` fails,
etc.


> On Nov. 16, 2015, 9:27 a.m., Joshua Cohen wrote:
> > pants.ini, line 19
> > <https://reviews.apache.org/r/40310/diff/2/?file=1125858#file1125858line19>
> >
> >     nit: fix indent?

You'll need to give me exact direction, -1 or +N or pull up onto the end of the prior line.
 There must be some whitespace indent per ConfigParser rules to keep this as a single value.


> On Nov. 16, 2015, 9:27 a.m., Joshua Cohen wrote:
> > pants.ini, line 77
> > <https://reviews.apache.org/r/40310/diff/2/?file=1125858#file1125858line77>
> >
> >     or maybe this identation style is intentional/correct?

Ah - yes, the indent is intentional / required.  The indent is required as explained above,
but the amount of indent is a style thing and your call.


> On Nov. 16, 2015, 9:27 a.m., Joshua Cohen wrote:
> > src/main/python/apache/aurora/client/api/__init__.py, lines 283-284
> > <https://reviews.apache.org/r/40310/diff/2/?file=1125860#file1125860line283>
> >
> >     alternately this should validate and save the interim var?
> >     
> >         return Restarter(
> >             job_key,
> >             updater_config,
> >             ...
> >             self._scheduler_proxy).restart(instances)

I couldn't follow the comment, but did change the style to the one you demonstrated.


- John


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40310/#review106674
-----------------------------------------------------------


On Nov. 15, 2015, 9:02 p.m., John Sirois wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40310/
> -----------------------------------------------------------
> 
> (Updated Nov. 15, 2015, 9:02 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Joe Smith, Maxim Khutornenko, and Zameer Manji.
> 
> 
> Bugs: AURORA-1532
>     https://issues.apache.org/jira/browse/AURORA-1532
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> This upgrades to pants 0.0.58 to pick up the newly split off pants
> python checks contrib plugin.  Release notes are here:
>   https://pypi.python.org/pypi/pantsbuild.pants/0.0.58
> 
> The plugin provides both python checkstyle (`compile.pythonstyle`), and
> a python eval task (`compile.python-eval`).  The `python-eval` is turned
> off since at least one of the Aurora python targets has files that have
> side-effects upon import (a repl is started).
> 
> Now style checks run before compile (and thus before tests) and they
> benefit from fingerprinting; ie: if you test your changes, those tests
> will run style checks and when you go to commit, those checks will not
> be re-run by the commit hook (although files you did not test will still
> need to be checked).
> 
> A few production files were fixed up according to style failures coming
> from:
> + no space after comment opening '#'
> + unused variables
> + mis-aligned hanging closing parens.
> 
> 
> Diffs
> -----
> 
>   build-support/hooks/pre-commit 619fa9e245be49e4e1f21781c0908cbf744b10ea 
>   build-support/jenkins/build.sh 41a392162f62236771ccbef5c9f94bf84b899f26 
>   build-support/python/checkstyle 61acc22613acece01580761b25afc7a3edb6b845 
>   build-support/python/checkstyle-check b2bfc5dd71193a8056828e9af05a4c16965f32a1 
>   pants.ini 319d38e9a7af8055cac5bbce4a6ae0cbb38dc8d0 
>   src/main/python/apache/aurora/admin/maintenance.py 6d94c923ae37bf6b827519d3505b100af306296b

>   src/main/python/apache/aurora/client/api/__init__.py 6f07a3073a5d422373238619d459fbd09d8adf3d

>   src/main/python/apache/aurora/client/cli/client.py 297fb588808c1eebc32ac3374265ba986dab3436

>   src/main/python/apache/aurora/client/cli/cron.py 6376fd014f2a4da29442b5c2c7eb36578b503ba3

>   src/main/python/apache/thermos/core/process.py fe95cb3be01b47616596bd78cb9a919b2e8bd978

>   src/main/python/apache/thermos/monitoring/process_collector_psutil.py f1ec5a9050ac60700c4a8afa905bcf12a9bd8a44

>   src/test/python/apache/aurora/admin/test_admin.py 8e204ab43c6bf69867ea7c32b0a7ba7fb29c0766

>   src/test/python/apache/aurora/admin/util.py 3570407b51613d0a7b4fde8a4794d88b98e150b5

>   src/test/python/apache/aurora/client/cli/test_task.py 5432a3d5f7e150b12bd75db0dac7a9018e1c6636

> 
> Diff: https://reviews.apache.org/r/40310/diff/
> 
> 
> Testing
> -------
> 
> See the discarded https://reviews.apache.org/r/40219/ for the
> commit-hook check.  This version of that RB engages the same code
> and this RB commit was vetted by the same commit-hook.
> 
> 
> Thanks,
> 
> John Sirois
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message