Status

P3
normal
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: Pike, Assigned: Pike)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(4 attachments, 1 obsolete attachment)

(Assignee)

Description

7 years ago
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.
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
Created attachment 549081 [details] [diff] [review]
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.
Attachment #549081 - Flags: review?(l10n)
(Assignee)

Comment 3

7 years ago
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?
Attachment #549081 - Flags: review?(l10n) → review+
Created attachment 549751 [details] [diff] [review]
check.py fixes for dashtags, accounts, l10nstats and webby
Attachment #549751 - Flags: review?(l10n)
(Assignee)

Comment 6

7 years ago
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.
Attachment #549751 - Flags: review?(l10n) → review-
(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.
Created attachment 549779 [details] [diff] [review]
Fixes the nits
Attachment #549751 - Attachment is obsolete: true
Attachment #549779 - Flags: review?(l10n)
(Assignee)

Comment 9

7 years ago
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?
Attachment #549779 - Flags: review?(l10n) → review+
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.
Created attachment 565955 [details] [diff] [review]
life app
Attachment #565955 - Flags: review?(l10n)
(Assignee)

Comment 13

7 years ago
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?
Attachment #565955 - Flags: review?(l10n) → review+
(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
Created attachment 591124 [details] [diff] [review]
All py files in app/shipping

...except tests.py which is under development in another branch
Attachment #591124 - Flags: review?(l10n)
(Assignee)

Updated

7 years ago
Attachment #591124 - Flags: review?(l10n) → review+
There are still management commands that haven't been checked.
Priority: -- → P3
(Assignee)

Comment 17

7 years ago
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.
Assignee: nobody → l10n
(Assignee)

Comment 18

7 years ago
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?
(Assignee)

Comment 20

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

Resolving FIXED, finally.
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
(Assignee)

Updated

6 years ago
Target Milestone: --- → 2.2
You need to log in before you can comment on or make changes to this bug.