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.
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.
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!
(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?
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. :)
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.
(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
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).
(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.
(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.
(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" :)
(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)?
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.
style-queue ran check-webkit-style on attachment 45299 [details] without any errors.
Created attachment 45300 [details] Proposed patch 2 Wrong file before.
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.
style-queue ran check-webkit-style on attachment 45300 [details] without any errors.
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?
I'm happy to land this manually once this patch is r+ed.
(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!
Comment on attachment 45300 [details] Proposed patch 2 Well, I'm certainly fine with the idea! Shinichiro is your man. :)
Committed r52436: <http://trac.webkit.org/changeset/52436>
By the way, thanks so much for re-organizing style code. Most of its mess came from me...