Bug 35316 - fix http_server --server start
: fix http_server --server start
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: New Bugs
: 528+ (Nightly build)
: Other Mac OS X 10.5
: P2 Normal
Assigned To: Dirk Pranke
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-02-23 15:40 PST by Dirk Pranke
Modified: 2010-02-24 17:54 PST (History)
4 users (show)

See Also:


Attachments
Patch (1.66 KB, patch)
2010-02-23 15:42 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff
revised patch - fix merge properly (3.55 KB, patch)
2010-02-23 16:40 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff
remove extraneous import from apache_http_server.py (3.12 KB, patch)
2010-02-23 16:43 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff
move __init__.get() into a new file (factory.py) (8.18 KB, patch)
2010-02-24 12:40 PST, Dirk Pranke
levin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Pranke 2010-02-23 15:40:30 PST
fix http_server --server start
Comment 1 Dirk Pranke 2010-02-23 15:42:23 PST
Created attachment 49332 [details]
Patch
Comment 2 Dirk Pranke 2010-02-23 16:40:55 PST
Created attachment 49342 [details]
revised patch - fix merge properly
Comment 3 Dirk Pranke 2010-02-23 16:43:04 PST
Created attachment 49343 [details]
remove extraneous import from apache_http_server.py
Comment 4 Dirk Pranke 2010-02-23 17:09:27 PST
Ojan, can you take a look at this and make sure it looks right?
Comment 5 Ojan Vafai 2010-02-23 17:39:45 PST
This seems sane to me. I don't really know how __init__ works though. Tony, does this look right to you?
Comment 6 Tony Chang 2010-02-23 17:57:02 PST
Is it possible to "import port" instead?  You may have to muck around with the include path.  Importing __init__ directly feels wrong.

The other way to do this is to move the get() method from __init__ into a different file (like base.py) and have __init__.py "import get from base".  Then you can use base.get from http_server.py.
Comment 7 Dirk Pranke 2010-02-23 18:00:22 PST
"import port" doesn't work w/o messing w/ the include path (and I don't want to do that because it feels really wrong).

"import get from base" might be the way to go.
Comment 8 Chris Jerdonek 2010-02-24 07:58:13 PST
(In reply to comment #6)
> Is it possible to "import port" instead?  You may have to muck around with the
> include path.  Importing __init__ directly feels wrong.
> 
> The other way to do this is to move the get() method from __init__ into a
> different file (like base.py) and have __init__.py "import get from base". 
> Then you can use base.get from http_server.py.

A few things--

(1) It looks like import __init__ is now listed twice.
(2) You shouldn't ever need to import __init__ directly.  Tony is right.  __init__ is what gets imported when you import the directory containing __init__.py.
(3) If you want to import a method inside __init__.py, then you can simply
"from port import get", for example.  I did this recently in the following patch:

https://bugs.webkit.org/show_bug.cgi?id=35162

Here I defined console_log_handler in webkitpy's __init__.py, so I was able to type "from webkitpy import console_log_handler" in a different file.
Comment 9 Chris Jerdonek 2010-02-24 08:03:36 PST
(In reply to comment #7)
> "import port" doesn't work w/o messing w/ the include path (and I don't want to
> do that because it feels really wrong).
> 
> "import get from base" might be the way to go.

You probably shouldn't need to modify sys.path.  A couple possibilities are to use relative imports:

from ..port import get

(I think this is the syntax for you in this case.  See here, for example--

http://docs.python.org/tutorial/modules.html#intra-package-references )

Or more preferable is to use absolute imports (according to PEP8).  Since webkitpy is already in your search I path I assume, you should just be able to do--

from webkitpy.layout_tests.port import get
Comment 10 Chris Jerdonek 2010-02-24 08:07:14 PST
(In reply to comment #8)
> A few things--
> 
> (1) It looks like import __init__ is now listed twice.

Sorry, I was looking at your first patch here, not your latest one.

You might also want to try--

from . import get

This might work to import get from your current directory's __init__.py, without having to use a relative import one level up (i.e. "from ..port").
Comment 11 Dirk Pranke 2010-02-24 12:38:27 PST
(In reply to comment #10)
> (In reply to comment #8)
> > A few things--
> > 
> > (1) It looks like import __init__ is now listed twice.
> 
> Sorry, I was looking at your first patch here, not your latest one.
> 
> You might also want to try--
> 
> from . import get
> 
> This might work to import get from your current directory's __init__.py,
> without having to use a relative import one level up (i.e. "from ..port").

Unfortunately, relative imports don't work in python 2.4, which is what Chromium uses on Windows. 

Moreover, I didn't particularly like the idea of importing the "get" symbol as an unqualified name. 

So, I moved the "get" into a new file (factory.py). I didn't move it into base.py to avoid the appearance of circular dependencies, and just to avoid confusing things.

Revised patch forthcoming.
Comment 12 Dirk Pranke 2010-02-24 12:40:17 PST
Created attachment 49422 [details]
move __init__.get() into a new file (factory.py)
Comment 13 David Levin 2010-02-24 16:34:20 PST
Comment on attachment 49422 [details]
move __init__.get() into a new file (factory.py)

Consider: Would be nice to have a little more in the changelog on a per file basis to explain why the changes were done in that file as discussed.
Comment 14 Dirk Pranke 2010-02-24 17:54:37 PST
Fix landed as r55210. Closing bug.