Import cleanup #520

Closed
wants to merge 9 commits into
from

Conversation

Projects
None yet
3 participants
Contributor

kkress commented Sep 8, 2012

Round 1: Fight

  • start the import cleanups with model. This puts model in the state it should be.
  • Remove all from r2.model import *
  • Fix up any modules that used from r2.model import * to make sure they weren't relying on an implicit import
  • This does not yet include support for __all__ @decorator, but that will come later.
  • Split builder.py into 2 files, builder.py contains the Builder base class and builders.py contains all the sub-classes. This was required to resolve Account and Subreddit circular dependencies.
  • Tested every import and removed any that were not needed for that file.
  • I have not yet touched function level imports. Those will be a separate change since those are likely to be a bit trickier dependency-wise.

Bonuses:

  • delete duplicate tup defintion
  • Added a pylintrc with some of reddit's coding standards (based on @kemitche's)
  • Any file touched has had its imports cleaned up a bit, removing any import *'s and relative import paths.
  • scraper.py now doesn't choke pylint (yes @spladug is going to get rid of it shortly, but I already modified it for import changes) this was done by removing the + at the end of each line, which was choking the parser for pylint

NOTES:

  • reddit_base.py : In MinimalController.post() near "update last_visit" there is a variable path_info that doesn't exist. I couldn't easily figure out what it should be from context, so I left it alone
  • template_helpers.py : There is a reference to non-existent class OrganicListing. This is one of about 5 I found, though i see no trace of that class. private maybe?
  • r2/controllers/ipn.py : verify_ipn() : Fixed the use of parameters, please verify this is correct
  • r2/controllers/promotecontroller.py : Fixed and issue in POST_edit_promo(), call near the end of the function changed to :

if getattr(link, "domain_override", False) or domain_override:

Should be:

if getattr(l, "domain_override", False) or domain_override:

Please verify

Testing Done:

  • pylint on all changed files
  • general poking at the site to ensure it worked as expected. Is there a script I can follow to ensure nothing is borked?

kkress added some commits Sep 6, 2012

@kkress kkress first round of import cleanups in model 9830f46
@kkress kkress based on comments, adding back in the function level imports. First r…
…ound will ignore those and do a sweep later to clean them up
a2f8a5e
@kkress kkress * r2/model is nearly complete. Still need to convert to using decerators
* r2/controllers no longer has any global imports
* r2/controllers is not in final state, but closer
* added missing r2admin prototype send_system_message to admintools
* fixed what looked like a typo in ipn.py : verify_ipn
* fixed what looked like a typo in promotecontroller.py : POST_edit_promo
2e6f8ee
@kkress kkress Second round of pylint based changes to primarily r2.lib to ensure th…
…at the

new model import changes don't cause other libraries that used to use model *
to break.  Who knows what imports they relied on before.
235f5b7

@kkress kkress commented on the diff Sep 8, 2012

r2/r2/config/utils.py
@@ -20,14 +20,6 @@
# Inc. All Rights Reserved.
###############################################################################
-iters = (list, tuple, set)
@kkress

kkress Sep 8, 2012

Contributor

This is the duplicate tup() definition. The real one is in r2.lib.utils

@bsimpson63 bsimpson63 commented on the diff Sep 8, 2012

r2/r2/controllers/ipn.py
@@ -122,7 +144,7 @@ def check_txn_type(txn_type, psl):
def verify_ipn(parameters):
- paraemeters['cmd'] = '_notify-validate'
+ parameters['cmd'] = '_notify-validate'
@bsimpson63

bsimpson63 Sep 8, 2012

Owner

Your typo correction is correct. Good (bad?) thing this function is never used!

kkress added some commits Sep 9, 2012

@kkress kkress Round 2 of import updates
* Added @export decorator
* Fixed @memoize and @wraps_api to use functools @wraps to ensure __name__ and
  __module__ are preserved
* refactored a few files to handle cycles
* added test_imports.py to allow 'compiling' all the python files and ensure
  they
* r2.config up to import snuff
* r2.controllers up to import snuff
* removed large import blocks and replaced with use of <module>.<symbol> style
* Fixed a lot of kw args with a key = value style to use key=value (only fixed
  where other change was needed)
* Started work on r2.lib, but not finished yet
25155c2
@kkress kkress Merge remote-tracking branch 'upstream/master' into import-cleanup
Conflicts:
	r2/r2/config/templates.py
	r2/r2/controllers/__init__.py
	r2/r2/controllers/api.py
	r2/r2/controllers/errors.py
	r2/r2/controllers/front.py
	r2/r2/controllers/promotecontroller.py
	r2/r2/controllers/validator/validator.py
	r2/r2/lib/app_globals.py
	r2/r2/lib/pages/pages.py
	r2/r2/lib/utils/utils.py
	r2/r2/models/builder.py
	r2/r2/models/listing.py
	r2/r2/models/modaction.py
	r2/r2/models/query_cache.py
	r2/r2/models/subreddit.py
	r2/r2/models/vote.py

merge and update the files in wiki changes to meet new standards
baddb7d
@kkress kkress Fixed a merge error I missed c881c9f
@kkress kkress Cleanup of round 2 changes.
A number of things were missed in the first set of import changes

* Fix a couple of new lib dirs that need more specific imports
* Fix a couple instances of things looking for NotFound in models
* Just export everything from pages.py, its simpler
c77d070

@kemitche kemitche and 1 other commented on an outdated diff Sep 10, 2012

r2/r2/controllers/api.py
-import csv
-from collections import defaultdict
-from datetime import datetime, timedelta
-import hashlib
-import urllib
-import urllib2
+from r2.controllers.api_docs import api_doc, api_section
+from r2.controllers.errors import errors
+from r2.controllers.oauth2 import OAuth2ResourceController, require_oauth2_scope
+from r2.controllers.reddit_base import (RedditController,
+ MinimalController,
+ set_user_cookie,
+ cross_domain,
+ paginated_listing,
+ )
+from r2.controllers.validator import (VAdmin,
@kemitche

kemitche Sep 10, 2012

Contributor

Obvious comment is obvious, but an alternate option for cases like these where there are a metric ton of imports is, of course, to import the module and use <module>.<Class>, e.g.,

from r2.controllers import validator
...
@validator.VAdmin
@kkress

kkress Sep 10, 2012

Contributor

I agree. In general it seems like using import XYZ style rather than from XYZ import foo might make more sense in the long run. For this first run I figured I'd see how it looks and get feedback.

In a later change I can do a full sweep of from style imports and try to replace all of them with named imports. Its likely that I'd also have to alias (not sure the right term for using import foo.bar.baz as baz) most of the imports to make them not ridiculously verbose.

@kemitche

kemitche Sep 10, 2012

Contributor

Fair enough. Taking it in stages is almost always better.

@kkress

kkress Sep 12, 2012

Contributor

So while doing this conversion I did find one advantage of using <package>.<symbol> imports instead of <module>.<symbol> or <package>.<module>.<symbol> with the package based ones since you are using from <module> import * at the package level you can only pull in items that are in __all__ (or using @export decorator)

@kemitche kemitche commented on the diff Sep 10, 2012

r2/r2/controllers/apiv1.py
@@ -21,9 +21,11 @@
###############################################################################
from pylons import c
+
+from r2.lib.jsontemplates import IdentityJsonTemplate
@kemitche

kemitche Sep 10, 2012

Contributor

What's the reasoning on this move? The alphabetical-ness was correct originally.

@kkress

kkress Sep 10, 2012

Contributor

See comment about the import ordering that I started using. basically I moved r2.lib above r2.controller because r2.controller was package local and r2.lib was an non-local package

Contributor

kemitche commented Sep 10, 2012

Still trawling through this, but one item of note on imports is that they should be alphabetized (within their groups) if possible; e.g.

import stdlib.aaaa
import stdlib.cccc

import thirdparty.aaaa
import thirdparty.bbbb

import r2.aaaa
import r2.bbbb.cccc
import r2.bbbb.fffff

@kemitche kemitche commented on the diff Sep 10, 2012

r2/r2/models/_builder.pyx
from r2.lib import utils
+from r2.models.builder import Builder, MAX_RECURSION, empty_listing
+from r2.models.link import (Comment,
+ Message,
+ MoreChildren,
+ MoreMessages,
+ MoreRecursion,
+ )
+
+__all__ = [
+ #Constants
@kemitche

kemitche Sep 10, 2012

Contributor

I really like how you've laid out __all__ w/ the comments 😄

@kkress

kkress Sep 10, 2012

Contributor

I considered adding the same comments to the from ... import statements, but left it out for now. With the discussion on removing from style it becomes sort of a moot point.

With the all stuff if I go forward with a decorator for exporting a symbol the comments would be reduced to just #Constants since that would be all that __all__ ever directly exports, with everything being implicit with @export

@kemitche kemitche and 1 other commented on an outdated diff Sep 10, 2012

r2/r2/controllers/promotecontroller.py
@@ -228,7 +257,7 @@ def POST_edit_promo(self, form, jquery, ip, l, title, url,
l.media_override = media_override
- if getattr(link, "domain_override", False) or domain_override:
+ if getattr(l, "domain_override", False) or domain_override:
@kemitche

kemitche Sep 10, 2012

Contributor

hrm. this is odd. Given that the body of the if is setting l.domain_override, the existing behavior may be correct, but I'm not sure where link is defined.

@kkress

kkress Sep 10, 2012

Contributor

Yeah I couldn't find a definition of link that fit (neither could pylint, btw) which is why I added the note.

@kemitche

kemitche Sep 10, 2012

Contributor

So, on current code, link is the r2.models.link module, which does not define domain_override. Which means this code was a bit broken in that I don't think it is currently possible to unset domain_override on an ad.

I think I'll try and pull this fix in sooner rather than later, so you may end up having rebase/merge issues at that point.

@kkress

kkress Sep 10, 2012

Contributor

No worries, I'll keep an eye out for the change and merge appropriately.

Contributor

kkress commented Sep 10, 2012

So I've been trying to apply the following rules to the imports as far as the ordering goes. Its mostly PEP-8 with 1 or 2 additional rules:

Package Groupings, each with a newline separator between groups

  1. python system packages
  2. 3rd party packages
  3. reddit codebase (non-local packages)
  4. reddit codebase (local packages)

for each Package Grouping import <package> are listed in alphabetical order. from <package> import <symbol> are then listed in alphabetical order and (though I've been less consistent on this one) the symbols imported are listed in 'symbol grouped' alphabetical order where the symbol groups are (in order):

  • Classes
  • Functions
  • Constants/Variables

Just to clarify this ends up looking like:

import os
import random
import sys
from os import file
from sys import module
...

@kkress kkress Fix pylint identified issues with imports
Mostly just missed <module> prefixes and 1 or 2 missed imports
57dccae
Contributor

kkress commented Oct 29, 2012

Too far out of date to be useful at this point. I'l see if I can salvage a few bits of it to be useful.

kkress closed this Oct 29, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment