674526 - check.py elmo

Status: RESOLVED FIXED

(Webtools Graveyard :: Elmo, defect, P3)

Description

[Axel Hecht [:Pike]]

We should make check.py mostly happy. Not religiously happy, but where it makes sense.

I guess doing that per app is a good idea, thus this should be a tracker, really.

Comment 1

[Peter Bengtsson [:peterbe]]

What I did with the Sheriff app (which is 99% check.py "correct") is that I installed kicker [1]
Then I ran:

    kicker -e 'check.py apps/users' apps/users
    ...edit edit edit...
    kicker -e 'check.py apps/cal' apps/cal
    
You start from the bottom and every time you make a save it re-runs the checker and gives you the next line to work on. 

[1] http://rubygems.org/gems/kicker

Comment 2

[Peter Bengtsson [:peterbe]]

Attachment: check.py fixes for privacy, homepage and bugsy

Bite wise doing a couple of apps. This bug won't be resolved by landing this patch.

Comment 3

[Axel Hecht [:Pike]]

Comment on attachment 549081 [details] [diff] [review]
check.py fixes for privacy, homepage and bugsy

Review of attachment 549081 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with what I think is a nit to be fixed below.

::: apps/privacy/tests.py
@@ +39,4 @@
>  from django.core.urlresolvers import reverse
>  from django.test import TestCase
>  from django.contrib.auth.models import User, Permission
> +from django.contrib.admin.models import LogEntry, ADDITION, CHANGE

I'd expect pyflakes to complain about ADDITION not being used? As I don't see it being used here, that is.

I suspect this is copy-n-paste from views.py?

Comment 4

[Peter Bengtsson [:peterbe]]

Bugsy, homepage and privacy done
https://github.com/mozilla/elmo/commit/cf0f40902c9aa94d1350e9104a96b1bc11d8c0f1

Comment 5

[Peter Bengtsson [:peterbe]]

Attachment: check.py fixes for dashtags, accounts, l10nstats and webby

Comment 6

[Axel Hecht [:Pike]]

Comment on attachment 549751 [details] [diff] [review]
check.py fixes for dashtags, accounts, l10nstats and webby

Review of attachment 549751 [details] [diff] [review]:
-----------------------------------------------------------------

We're getting there, sweet. r- because the test precision and the mix of autoescape feel more like an r- than an r+ with nits.

::: apps/dashtags/templatetags/recurse.py
@@ +74,2 @@
>  class DepthNode(template.Node):
> +

This empty line sneaked in?

::: apps/dashtags/tests.py
@@ +30,5 @@
>                ]}
>          c = template.Context(d)
>          out = t.render(c)
> +        self.assertEqual(out.strip(),
> +                         u'root\n\n leaf1\n\n  leafleaf1\n\n\n leaf2')

Please don't strip(), that makes the test fail for some whitespace and not fail for other.

::: apps/l10nstats/models.py
@@ +160,4 @@
>          managed = False
>  
> +    run = models.ForeignKey(Run)
> +    changeset = models.ForeignKey(Changeset)

I think we should keep Meta at the end of the class definition, that's how the django docs do it, and it makes more sense to me, too.

::: apps/l10nstats/templatetags/run_filters.py
@@ +50,2 @@
>  @register.filter
>  def showrun(run, autoescape=None):

As you're killing the boilerplate implementation of autoescape, I think the right way to fix the complaints about autoescape not being used is to not do autoescape.

Can you kill the optional argument, and the 

showrun.needs_autoescape = True

(not seen in the patch) and just add a comment that we're not taking input strings, and thus don't need to do anything about escaping the input?

I've been re-reading https://docs.djangoproject.com/en/1.3/howto/custom-template-tags/#filters-and-auto-escaping, and that's what I understand that to say today.

Comment 7

[Peter Bengtsson [:peterbe]]

(In reply to comment #6)
> Comment on attachment 549751 [details] [diff] [review] [diff] [details] [review]
> check.py fixes for dashtags, accounts, l10nstats and webby
> 
> Review of attachment 549751 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> We're getting there, sweet. r- because the test precision and the mix of
> autoescape feel more like an r- than an r+ with nits.
> 
> ::: apps/dashtags/templatetags/recurse.py
> @@ +74,2 @@
> >  class DepthNode(template.Node):
> > +
> 
> This empty line sneaked in?
> 
Deliberate.

> ::: apps/dashtags/tests.py
> @@ +30,5 @@
> >                ]}
> >          c = template.Context(d)
> >          out = t.render(c)
> > +        self.assertEqual(out.strip(),
> > +                         u'root\n\n leaf1\n\n  leafleaf1\n\n\n leaf2')
> 
> Please don't strip(), that makes the test fail for some whitespace and not
> fail for other.
> 
Ok. Trimming removed. 

> ::: apps/l10nstats/models.py
> @@ +160,4 @@
> >          managed = False
> >  
> > +    run = models.ForeignKey(Run)
> > +    changeset = models.ForeignKey(Changeset)
> 
> I think we should keep Meta at the end of the class definition, that's how
> the django docs do it, and it makes more sense to me, too.
> 
My bad. I got it the wrong way around. The docs are here by the way:
https://docs.djangoproject.com/en/dev/internals/contributing/writing-code/coding-style/

> ::: apps/l10nstats/templatetags/run_filters.py
> @@ +50,2 @@
> >  @register.filter
> >  def showrun(run, autoescape=None):
> 
> As you're killing the boilerplate implementation of autoescape, I think the
> right way to fix the complaints about autoescape not being used is to not do
> autoescape.
> 
> Can you kill the optional argument, and the 
> 
> showrun.needs_autoescape = True
> 
> (not seen in the patch) and just add a comment that we're not taking input
> strings, and thus don't need to do anything about escaping the input?
> 
> I've been re-reading
> https://docs.djangoproject.com/en/1.3/howto/custom-template-tags/#filters-
> and-auto-escaping, and that's what I understand that to say today.

Will do.

Comment 8

[Peter Bengtsson [:peterbe]]

Attachment: Fixes the nits

Comment 9

[Axel Hecht [:Pike]]

Comment on attachment 549779 [details] [diff] [review]
Fixes the nits

Review of attachment 549779 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the remaining .strip() fixed.

::: apps/dashtags/tests.py
@@ +30,5 @@
>                ]}
>          c = template.Context(d)
>          out = t.render(c)
> +        self.assertEqual(out.strip(),
> +                         u'root\n\n leaf1\n\n  leafleaf1\n\n\n leaf2')

The strip() is still here, fix that, too?

Comment 10

[Peter Bengtsson [:peterbe]]

Landed https://github.com/mozilla/elmo/commit/54f0318c14d2548eb108e9687a663d0940cec50f
This time without the `.strip()` stuff.

Comment 11

[Laura Thomson :laura]

Last outstanding app is shipping, Pike will ping Peter when it's ready to run.  This will be after all the model-wrangling is complete.

Comment 12

[Peter Bengtsson [:peterbe]]

Attachment: life app

Comment 13

[Axel Hecht [:Pike]]

Comment on attachment 565955 [details] [diff] [review]
life app

Review of attachment 565955 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the comment addressed.

::: apps/life/admin.py
@@ +1,1 @@
> +from .models import Tree, Repository, Locale, Forest

Let's not have a new code style?

Comment 14

[Peter Bengtsson [:peterbe]]

(In reply to Axel Hecht [:Pike] from comment #13)
> Comment on attachment 565955 [details] [diff] [review] [diff] [details] [review]
> life app
> 
> Review of attachment 565955 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> r=me with the comment addressed.
> 
> ::: apps/life/admin.py
> @@ +1,1 @@
> > +from .models import Tree, Repository, Locale, Forest
> 
> Let's not have a new code style?
Ok. Dot removed. 

patch landed
https://github.com/mozilla/elmo/commit/84e63b0b950b6a4b5150d51789e7b8f2b2e1de22

Comment 15

[Peter Bengtsson [:peterbe]]

Attachment: All py files in app/shipping

...except tests.py which is under development in another branch

Comment 16

[Peter Bengtsson [:peterbe]]

There are still management commands that haven't been checked.

Comment 17

[Axel Hecht [:Pike]]

Taking this for a bit, as I want to get a few code modules clean so that I can hack on them, notably the build model and status stuff.

Comment 18

[Axel Hecht [:Pike]]

I've put the remaining patches but 1 or two complaints onto this branch: https://github.com/Pike/elmo/compare/mozilla:develop...Pike:feature/check-tinder. I excluded apps/commons, and static (which is full of exhibit).

peterbe, want to review that over there?

Comment 19

[Axel Hecht [:Pike]]

It's also https://github.com/mozilla/elmo/pull/3

Comment 20

[Axel Hecht [:Pike]]

https://github.com/mozilla/elmo/commit/057f4de0cd38ded755fdfc460be9a8e183685faa merges that branch.

Resolving FIXED, finally.