Bug 59908 – scm.py should be split into many pieces

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Eric Seidel (no email)]

scm.py should be split into many pieces

[Eric Seidel (no email)]

Created attachment 91866 [details]
Patch

[Eric Seidel (no email)]

Committed r85450: <http://trac.webkit.org/changeset/85450>

[Dirk Pranke]

Comment on attachment 91866 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=91866&action=review

> Tools/Scripts/webkitpy/common/checkout/scm/__init__.py:7
> +from .svn import SVN

I mentioned this in the other bug, but since you seem to be doing this intentionally here, relative path imports are discouraged in PEP-8.

> Tools/Scripts/webkitpy/common/checkout/scm/__init__.py:34
> +    """

We should really add a comment here that this routine should NOT be used to determine the top of the webkit checkout, since that will only work if (a) the CWD is actually in the webkit checkout and not some other checkout and (b) the webkit checkout is a full checkout and not something like Chromium's hand-picked subset. We should point to a suggested alternative.

[Adam Barth]

(In reply to comment #3)
> (From update of attachment 91866 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=91866&action=review
> 
> > Tools/Scripts/webkitpy/common/checkout/scm/__init__.py:7
> > +from .svn import SVN
> 
> I mentioned this in the other bug, but since you seem to be doing this intentionally here, relative path imports are discouraged in PEP-8.

That's what I thought, but Eric kept saying otherwise.

[Eric Seidel (no email)]

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 91866 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=91866&action=review
> > 
> > > Tools/Scripts/webkitpy/common/checkout/scm/__init__.py:7
> > > +from .svn import SVN
> > 
> > I mentioned this in the other bug, but since you seem to be doing this intentionally here, relative path imports are discouraged in PEP-8.
> 
> That's what I thought, but Eric kept saying otherwise.

I haven't read pep-8 recently, so I don't feel an expert here.

I don't think that absolute paths make sense inside module containers inside webkitpy.  I think our deep module tree makes things tricky.

I don't think .. relative paths *ever* make sense.

But I would like to tighten up our module boundaries more.  We have problems of one module reaching out into another module (checkout.changelog reaching out to bugzilla and bugzilla reaching back into checkout.scm.git for example) which I would like to get rid of.

It feels a bit strange to me to have scm/__init__.py need to know where it is just to import its own .scm.py file, but if that's what the python experts tell us we should do, I'm not sure I know enough to disagree.