Bug 32773 – [check-webkit-style] Create folder in Scripts/modules for style-related files

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Chris Jerdonek]

Create a "styles" folder in Scripts/modules, and move cpp_style.py, text_style.py, and their unit tests into the folder as cpp.py, text.py, etc.

This will also give us a location for new style types and common.py as they become necessary.

[Eric Seidel (no email)]

We could also consider just making it a peer of modules instead of inside modules.  modules probably needs a better name.

Either way sounds good to me. modules/* has gotten too big.

[Eric Seidel (no email)]

I also might consider naming it webkit_style/ or style/ instead of styles/ donno.  In any case, I'm glad you're hacking on the style code!

[Chris Jerdonek]

(In reply to comment #1)
> We could also consider just making it a peer of modules instead of inside
> modules.  modules probably needs a better name.

Are there any conventions in the Scripts folder (e.g. dependencies, etc) that we're trying to maintain that I might not know about?  Is there any structure right now?

[Eric Seidel (no email)]

Scripts is almost entirely individual perl scripts.  There are two perl modules we share between some of them "webkitdirs.pm" and "VCSUtils.pm".

When I started bugzilla-tool, I wanted to break it up into lots of class files/modules, so I created "modules/" and dumped everything in there.  When Hamaji/Dave imported cpp_style.py from Chromium's repository, that also went into modules/.  Now that we're getting more and more python code, it makes sense to break things up a bit more.

So in short, no we don't really have any conventions to follow here.  Welcome to the wild west. :)

[Eric Seidel (no email)]

Adam and I as recently as last night we discussing breaking some of the modules in modules/ out into separate or sub-modules.

Something to hold scm, bugzilla, executive, grammar, buildbot

Something else to hold all the comamnds/tool stuff

Something else to hold all the webkit specific config (like committers, changelogs, etc.)

So yeah, we're attempting to invent nice conventions for the future of python in WebKit.  Soon (within weeks of the new year) all of the run_webkit_tests.py modules will be upstreamed, and they'll probably go in their own directory as well.

[Chris Jerdonek]

(In reply to comment #2)
> I also might consider naming it webkit_style/ or style/ instead of styles/
> donno.  In any case, I'm glad you're hacking on the style code!

Sure thing.  I was thinking that styles/ would only be for the file types and
not the main style.py.  So the main style.py would import styles.cpp,
styles.text, etc.

If we wanted a folder for all the style-related stuff, then I'd probably want
two levels.  Something like (though not sure yet what webkit_style/ and
style.py should be called)--

webkit_style/
    style.py
    styles/
        cpp.py
        text.py

[Eric Seidel (no email)]

Is styles.py necessary in your example, or can it be covered by the __init__.py?
http://docs.python.org/tutorial/modules.html
(warning, I'm relatively new to this python thing).

[Chris Jerdonek]

(In reply to comment #5)
> Adam and I as recently as last night we discussing breaking some of the modules
> in modules/ out into separate or sub-modules.
> 
> Something to hold scm, bugzilla, executive, grammar, buildbot

If you're going to be doing some removes/renames, I know that Darin had some opinions about using the name "bugzilla":

https://bugs.webkit.org/show_bug.cgi?id=32542#c3

This might be a good time to consider a rename -- if you agree.

> So yeah, we're attempting to invent nice conventions for the future of python
> in WebKit.  Soon (within weeks of the new year) all of the run_webkit_tests.py
> modules will be upstreamed, and they'll probably go in their own directory as
> well.

Something else to think about.  It might be nice if, for example, run-webkit-unittests didn't need to get updated each time a new file was added to the style code.  So perhaps in the new scheme each module like the style-checker could have a single master unit test file that run-webkit-unittests would import.

[Chris Jerdonek]

(In reply to comment #7)
> Is styles.py necessary in your example, or can it be covered by the
> __init__.py?

Yeah, it's necessary because it contains a fair amount of code in its own right with its own unittest file, etc.  It's the file that contains the logic to parse and validate arguments, and it acts as a dispatcher from the command-line wrapper to the different language parsers.  Over time it could potentially split into more than file.

I don't necessarily think it should be called style.py.  I just don't have a good name yet in mind.  Maybe style_checker.py.

[Eric Seidel (no email)]

(In reply to comment #8)
> If you're going to be doing some removes/renames, I know that Darin had some
> opinions about using the name "bugzilla":
> 
> https://bugs.webkit.org/show_bug.cgi?id=32542#c3

His comments appear unrelated.  bugzilla.py is the module which knows how to talk to a bugzilla instance.  We could create a bugs.py with an abstract interface if we ever wanted to have a radar.py (apple's bug database) or some other bug database.  tool.bugs was named bugs instead of bugzilla with exactly this in mind.

> Something else to think about.  It might be nice if, for example,
> run-webkit-unittests didn't need to get updated each time a new file was added
> to the style code.  So perhaps in the new scheme each module like the
> style-checker could have a single master unit test file that
> run-webkit-unittests would import.

This is easily solved using __ALL__ in the __init__.py files.  Adam and I just haven't quite gotten there yet.  Our python-fu is strong, but not quite that strong yet. :)

I believe we could just use from modules import * and it would all "just work" :)

[Chris Jerdonek]

(In reply to comment #1)
> We could also consider just making [the style folder] a peer of 
> modules instead of inside modules.

(And comment #5)
> Adam and I as recently as last night we discussing breaking some 
> of the modules in modules/ out into separate or sub-modules.

After thinking about this, it seems like it might be better to keep all of the "library" python code under a single folder (but grouped into subfolders and packages) -- as opposed to having some of it be in siblings of the modules folder.

This would make it easier to locate and reference other python modules.  It also keeps the top level Scripts folder cleaner and easier to navigate.  Since the Scripts folder contains the scripts executed by the end-user, people might be browsing its contents:

http://webkit.org/coding/scripts.html

Are there any reasons we might want to have multiple folders of python code in the top level of the Scripts folder?

(In reply to comment #1)
> modules probably needs a better name.

What are some of the names you were considering?  I'm starting to think maybe we can call it pywebkit, or simply webkit.  Then from a top-level script, you could write, for example--

import webkit.style.checker
import webkit.scm....

Finally, a couple questions just to clarify:

In terms of restructuring, I assume it's safe to say we want to continue storing all of the end-user scripts in a single folder?

Are any of the scripts currently in the top level Scripts folder not meant to be executed by a human being (i.e. meant more to be called from other scripts)?

[Chris Jerdonek]

Created attachment 45299 [details]
Proposed patch

I'm not sure how to proceed with this since it doesn't look like svn-create-patch supports directory adds.

I would like a directory named "style" added to Scripts/modules.  The style-related files can be moved/renamed in a subsequent patch.

After the comment discussion with Eric, it definitely seems worth separating all of the style-related files from the other files in the modules folder, and not just the style-related files having to do with file types (cpp_style.py, text_style.py, etc).  And because there are only around 6 style-related files in all, it doesn't seem necessary to create an additional sub-folder inside for the file types.  That can always be done later, if necessary.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45299 [details] without any errors.

[Chris Jerdonek]

Created attachment 45300 [details]
Proposed patch 2

Wrong file before.

[Eric Seidel (no email)]

Well, we could fix svn-create-patch, svn-apply, etc. to support all this. :)  But yes, we need to just make you a committer and then problems like this are easier to solve.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45300 [details] without any errors.

[Eric Seidel (no email)]

I think we should wait and do this once you're a committer.  Unless you're asking for someone to create the directory for you so that you can cq the rest of the patches?

[Shinichiro Hamaji]

I'm happy to land this manually once this patch is r+ed.

[Chris Jerdonek]

(In reply to comment #17)
> I think we should wait and do this once you're a committer.  Unless you're
> asking for someone to create the directory for you so that you can cq the rest
> of the patches?

Yeah, that's what I had in mind.  It looks like svn-create-patch supports
moving/renaming files.

(In reply to comment #17)

Thanks, Shinichiro!

[Eric Seidel (no email)]

Comment on attachment 45300 [details]
Proposed patch 2

Well, I'm certainly fine with the idea!  Shinichiro is your man. :)

[Shinichiro Hamaji]

Committed r52436: <http://trac.webkit.org/changeset/52436>

[Shinichiro Hamaji]

By the way, thanks so much for re-organizing style code. Most of its
mess came from me...