Improve properties.mako.rs file structure #10586

perlun · 2016-04-13T21:03:56Z

Background: when I started hacking on Servo some week(s) ago, I quite quickly came in contact with the components/style/properties.mako.rs file. I was surprised to see that it was so large (7000 lines of code). In my book, that's too much; there must clearly be some way to structure things in a more reasonable way, I feel.

I am by no means a Python nor a Mako expert (more like: I've never used it before experiencing it in Servo), but I started googling about it yesterday and saw that Mako has an %include system that could be used for this kind of stuff. Said and done, I started playing around with it and managed to get something that compiles today - I have used this approach now for extracting a set of properties from the big file.

I realize this is a significant change to the structure here, and this PR serves more as a starting point for discussion than anything else. There may be other, superior ways of achieving the same end result (i.e. splitting the file into smaller parts), better timing for trying to land stuff like this etc etc. Having that said, I find the way I chose here pretty decent to be honest. The properties.mako.rs is down from ~7000 LoC to about 5800, by extracting 24 properties into separate files. There are about 70 properties left (of which some hard more difficult, because of scoping etc - they use local methods declared in the properties.mako.rs file which are un-callable from include files (have to use %def but it didn't seem trivial when I started looking at that, so skipped those properties for now). So, all in all, I think we should be able to bring down this 7000 line file to maybe 2-3000 lines or something quite easily, which would IMHO be a great start.

Opinions/flames/etc? 😎

This change is

highfive · 2016-04-13T21:04:00Z

Heads up! This PR modifies the following files:

@bholley: components/style/properties/outline-width.mako.rs, components/style/properties/font-weight.mako.rs, components/style/properties/quotes.mako.rs, components/style/properties/line-height.mako.rs, components/style/properties/border-widths.mako.rs, components/style/properties/overflow-y.mako.rs, components/style/properties/float.mako.rs, components/style/properties/background-size.mako.rs, components/style/build_properties_rs.py, components/style/properties/font-family.mako.rs, components/style/properties/servo-display-for-hypothetical-box.mako.rs, components/style/properties/word-spacing.mako.rs, components/style/properties/background-image.mako.rs, components/style/properties/text-decoration.mako.rs, components/style/properties/content.mako.rs, components/style/properties.mako.rs, components/style/properties/list-style-image.mako.rs, components/style/properties/color.mako.rs, components/style/properties/letter-spacing.mako.rs, components/style/properties/outline-style.mako.rs, components/style/build.rs, components/style/properties/counter-increment.mako.rs, components/style/properties/font-size.mako.rs, components/style/properties/counter-reset.mako.rs, components/style/properties/text-align.mako.rs, components/style/properties/z-index.mako.rs, components/style/properties/background-position.mako.rs

highfive · 2016-04-13T21:04:01Z

Warning

These commits modify style code, but no tests are modified. Please consider adding a test!

perlun · 2016-04-13T21:05:16Z

components/style/build.rs

-except:
-    sys.stderr.write(exceptions.text_error_template().render().encode('utf8'))
-    sys.exit(1)
-"#)


This change is not mandatory in the PR, but it felt natural in line with the thinking to "avoid multiple languages in the same file" (can't find the reference now...). Maintaining Python code is also easier when it has a proper .py suffix etc.

Can you fix this in geckolib too?

KiChjang · 2016-04-13T21:06:59Z

Oh yes, this is definitely a nice change! I believe the idea was to actually do away with mako altogether and use native Rust macros to generate code, but this is an interim state that is still desirable, given that properties.mako.rs is bloated as it is right now. I had a thought that the directory structure of style should really be structured like script a few days ago, but this looks great!

perlun · 2016-04-13T21:09:30Z

Yeah, I also thought about native Rust macros (which would be the natural approach if possible). But, maybe that's more a long-term goal and trying to "clean up the current mess" can be the short-term fix.

bholley · 2016-04-13T22:10:37Z

Yeah, I would be pretty opposed to getting rid of the mako stuff right now. It's extremely flexible and very understandable for non-experts, both of which are crucial for the Stylo work right now. Everybody complains about it, but I haven't heard a cogent plan for how we would do all these things concisely via the AST, or why it would be better.

As for this PR (splitting out properties into separate files): there are pros and cons, but I think it's probably a net win (assuming we follow through with it). As long as I can still import all the data structures from geckolib, I'm happy. I'll let @SimonSapin decide though. :-)

SimonSapin · 2016-04-13T22:40:18Z

@bholley Something based on a syntax extension would allow us to have Rust error messages use line numbers from source files rather than a generated file. But maybe rust-lang/rfcs#1573 (source maps) will eventually be another way to do that.

I agree with the general idea of this PR: properties.mako.rs is too big, and replacing Mako is not gonna happen soon. But I’d like a more systematic way to decide what goes into what files. One file per property is probably too many files. Maybe one file per style struct?

I think we can separate even more data definition from code generation. I’ll look in more details tomorrow.

KiChjang · 2016-04-14T00:53:01Z

$ ./mach test-unit
Traceback (most recent call last):
  File "/home/travis/build/servo/servo/components/style/list_properties.py", line 16, in <module>
    template.render(PRODUCT='servo')
  File "/home/travis/build/servo/servo/components/style/Mako-0.9.1.zip/mako/template.py", line 443, in render
  File "/home/travis/build/servo/servo/components/style/Mako-0.9.1.zip/mako/runtime.py", line 807, in _render
  File "/home/travis/build/servo/servo/components/style/Mako-0.9.1.zip/mako/runtime.py", line 839, in _render_context
  File "/home/travis/build/servo/servo/components/style/Mako-0.9.1.zip/mako/runtime.py", line 865, in _exec_template
  File "_home_travis_build_servo_servo_components_style_properties_mako_rs", line 225, in render_body
  File "/home/travis/build/servo/servo/components/style/Mako-0.9.1.zip/mako/runtime.py", line 730, in _include_file
  File "/home/travis/build/servo/servo/components/style/Mako-0.9.1.zip/mako/runtime.py", line 770, in _lookup_template
mako.exceptions.TemplateLookupException: Template '/home/travis/build/servo/servo/components/style/properties.mako.rs' has no TemplateLookup associated
Error running mach:
    ['test-unit']
The error occurred in code that was called by the mach command. This is either
a bug in the called code itself or in the way that mach is calling it.
You should consider filing a bug for this issue.
If filing a bug, please include the full output of mach, including this error
message.
The details of the failure are as follows:
CalledProcessError: Command '['/home/travis/virtualenv/python2.7.10/bin/python2.7', u'/home/travis/build/servo/servo/components/style/list_properties.py']' returned non-zero exit status 1
  File "/home/travis/build/servo/servo/python/servo/testing_commands.py", line 146, in test_unit
    path.join(self.context.topdir, "components", "style", "list_properties.py")
  File "/opt/python/2.7.10/lib/python2.7/subprocess.py", line 573, in check_output
    raise CalledProcessError(retcode, cmd, output=output)
The command "./mach test-unit" exited with 1.

perlun · 2016-04-14T05:38:40Z

Thanks to you all for the feedback thus far!

@bholley - As for extracting the Mako scripts to .py for geckolib, yes sure - I can fix this.
@KiChjang - the unit test failures; thanks for pointing that out, will look into that as well.

Regarding the structure (how to split, one file per property or not), that's definitely one of the harder questions. I don't really have a full understanding of the structure here yet; one file per property didn't feel completely unreasonable (yes, it gives us many files but OTOH we can put the property name in the file name making it quite easy to find what you're looking for). But absolutely, if we can agree on a better, more balanced structure, I'm all open for suggestions. (@SimonSapin, to make it even clearer - please suggest a few of the file names so I get your general idea of the overall picture)

perlun · 2016-04-14T13:45:27Z

FWIW, here are the file lengths of the new files introduced in the PR. I don't know what the recommended "reasonable" maximum class/code file length is in Rust-land? In the domain I normally work (Ruby), 100 lines is considered a good maximum for a class. Do we have a recommendation yet in the Rust community?

$ wc -l properties/*
      61 properties/background-image.mako.rs
     114 properties/background-position.mako.rs
     137 properties/background-size.mako.rs
      39 properties/border-widths.mako.rs
      37 properties/color.mako.rs
     164 properties/content.mako.rs
      70 properties/counter-increment.mako.rs
      10 properties/counter-reset.mako.rs
      20 properties/float.mako.rs
     110 properties/font-family.mako.rs
      66 properties/font-size.mako.rs
     119 properties/font-weight.mako.rs
      63 properties/letter-spacing.mako.rs
      93 properties/line-height.mako.rs
      66 properties/list-style-image.mako.rs
      15 properties/outline-style.mako.rs
      33 properties/outline-width.mako.rs
      39 properties/overflow-y.mako.rs
      67 properties/quotes.mako.rs
      17 properties/servo-display-for-hypothetical-box.mako.rs
      53 properties/text-align.mako.rs
      87 properties/text-decoration.mako.rs
      63 properties/word-spacing.mako.rs
      47 properties/z-index.mako.rs
    1590 total

SimonSapin · 2016-04-14T13:54:01Z

I don’t think there is such a convention in Rust. (FWIW 100 lines seems very small to me, I don’t understand why Rubyists considers that a maximum.)

bholley · 2016-04-14T16:08:45Z

Yeah, doing it per-style-struct seems sensible IMO.

bors-servo · 2016-04-14T17:29:53Z

☔ The latest upstream changes (presumably #10556) made this pull request unmergeable. Please resolve the merge conflicts.

This breaks out some of the parts on servo#10586, that should be easily mergeable. The idea would be to let you review & merge it first, and then I'll complete the other PR rebase off of this stuff.

perlun · 2016-04-14T20:46:34Z

Yeah, I know 100 lines is a bit extreme 😄 but it actually works quite well in the Ruby case; it's a pretty expressive language that has good mechanics in place to help keeping classes short (then again, sometimes it's just not possible). I think the general idea of having some rule or recommendation within a language community makes sense; it gives new developers also a feeling of "what is reasonable" - to indicate signs of code smell, and when stuff should be acted upon. Even Microsoft had this in the C# days I think (googled it a bit couldn't find the specifics), although their rule was probably a lot more than 100 LoC... 😄

Anyway, Rust is not Ruby and perhaps we shouldn't get sidetracked too much. I looked a bit at the file again today, trying to understand your POV better @bholley and @SimonSapin - you were speaking about one file per style struct. Are you talking about these modules?

pub mod longhands
pub mod shorthands
pub mod style_struct_traits
pub mod style_structs

I.e. to split it in four pieces instead. Was that what you were suggesting? Or was it something else?

jdm · 2016-04-14T22:27:33Z

I believe the suggestion was to split it at the level of each new_style_struct invocation: border.rs.mako, padding.rs.mako, etc.

bholley · 2016-04-14T23:04:51Z

@jdm is correct. Note that the current scheme jumps around a bit with the switch_to_style_struct stuff. That would presumably be consolidated.

@bholley

…iles, r=bholley Extracted Mako-based code generation invokation to separate .py files. This breaks out some of the parts on #10586, that should be easily mergeable (hopefully pretty much a no-brainer really). The idea would be to let you review & merge it first, and then I'll complete the other PR rebase off of this stuff. @bholley - I did like you suggested and broke it out for `geckolib` as well. The tests should also be running without problems (tested `./mach test-unit` locally).  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/10617)

perlun · 2016-04-15T05:38:18Z

Alright, that makes sense. Will give it a try and carve something out based on these suggestions (but probably only for a small subset of the file first to ensure I get it right). Thanks.

SimonSapin · 2016-04-15T09:27:51Z

I believe I originally introduced switch_to_style_struct to keep properties roughly in the order of CSS 2.1 chapters, but that is not really important. We can get rid of it by moving property definitions around (which is happening anyway if they move to other files).

This breaks out some of the parts on servo#10586, that should be easily mergeable. The idea would be to let you review & merge it first, and then I'll complete the other PR rebase off of this stuff.

perlun · 2016-04-18T21:03:36Z

Ouch man, that hurts... 😃 I guess I should have checked my inbox before spending like the 3rd night or something on this, trying to accommodate to the suggestions given by you... 😉 It was really, really hard to be able to get the include stuff working but I finally managed to get it done. It feels good, but I had to learn a bit more about Mako than I'd really have wished for to be able to get it done.

So, here goes: what I have at the moment is pushed to this branch. Please check it out if you like. As stated, I wasn't aware of the work you'd done on this until now, when I pushed this up and read your message. 😇

Having that said: I don't take pride in forcing my way through here. Your changes are really good, from what I understand of them: trying to structure things in a bit better way. I think I have most of my changes well under control, so even though it will be awful to having to rebase it to merge with your stuff... well... Either that way, or you allow this stuff through first, and then rebase your branch on top off that once it's landed in master. Given that you did in fact snatch my PR, it wouldn't be entirely insane to suggest that... 😉

(to be very clear, humour aside: no hard feelings towards you whatsoever, really. I do appreciate your work in this area, especially since I know how awkward it can be to work with the Python/Mako stuff. Yes, I know it very very well indeed. 😛)

(final PS: I realize you perhaps thought the PR was going stale or something. The reason why it took so long time was simply that it was very hard to get progress in the way you suggested.)

perlun · 2016-04-18T21:06:28Z

components/style/list_properties.py

+lookup = TemplateLookup(directories=[style])
+template = Template(filename=os.path.join(style, "properties.mako.rs"),
+                    input_encoding='utf8',
+                    lookup=lookup)


These changes are needed to be able to use %include. It was significantly harder to be able to use it from list_properties.py, so in that sense your (Simon) change in the other branch where you refactor list_properties.py away is kind of nice.

SimonSapin · 2016-04-18T21:16:02Z

No, I didn’t think this PR was stale and did realize that you might still be working on this. I should have communicated better from the start, sorry. I’ll look tomorrow at your latest changes and try to integrate both branches.

perlun · 2016-04-18T21:26:09Z

Sounds great! G'night for now.

perlun · 2016-04-18T21:33:19Z

(I noted there were a bunch of linting/tidy issues. Feel free to fork my branch and fix them if you like, if you want to speed the integration of these changes. Otherwise I'll try to sort them out tomorrow. I'd unfortunately only verified that this built, that geckolib built, that the unit tests ran... but hadn't verified the tidy checks before pushing...)

perlun · 2016-04-19T04:38:20Z

Tidiness checks should now be OK. Please check and let me know if we can merge as-is or if you want it squashed up a bit.

perlun · 2016-04-19T13:59:22Z

@SimonSapin - any progress on this one? I'm eager to continue moving stuff out of the monster file... 😄

SimonSapin · 2016-04-19T16:14:36Z

I’m in the middle of responding to 34 review comments on another PR. This is next :)

perlun · 2016-04-19T19:08:57Z

I’m in the middle of responding to 34 review comments on another PR.

Oh wow! I don't envy you. 😉

@nox

Prepare related files to make it easier to split up the Mako template #10586 (comment) r? @nox  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/10749)

@nox

Prepare related files to make it easier to split up the Mako template #10586 (comment) r? @nox  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/10749)

This is a new attempt of servo#10586, after Simon Sapin's great cleanups in servo#10749 has landed. I have adjusted the changes to the new structure that was introduced, and also only done a few of the longhand ones. Will certainly continue on this as soon as we have a basic agreement that this style is reasonable.

perlun · 2016-04-20T21:09:52Z

Closing this now Simon - I have a new PR coming up, but I'm unable to submit it until #10749 is landed (since I branched off of that branch; my branch now includes all your commits until I'm able to rebase it on top of master once homu/bors/buildbots have done their work). Thanks a lot for what you've done so far! Sorry if I sounded a bit rude the other night; it was kind of annoying yes, but... I think the end result is a much nicer codebase in these parts, which is something which we will all benefit from in the long run.

@nox

Prepare related files to make it easier to split up the Mako template #10586 (comment) r? @nox  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/10749)

@nox

Prepare related files to make it easier to split up the Mako template #10586 (comment) r? @nox  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/10749)

This is a new attempt of servo#10586, after Simon Sapin's great cleanups in servo#10749 has landed. I have adjusted the changes to the new structure that was introduced, and also only done a few of the longhand ones. Will certainly continue on this as soon as we have a basic agreement that this style is reasonable.

…Sapin Improve properties.mako.rs file structure, take 2 This is a new attempt of #10586, after Simon Sapin's great cleanups in #10749 has landed. I have adjusted the changes to the new structure that was introduced, and also only done a few of the longhand ones. Will certainly continue on this as soon as we have a basic agreement that this style is reasonable.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/10774)

This is a new attempt of servo#10586, after Simon Sapin's great cleanups in servo#10749 has landed. I have adjusted the changes to the new structure that was introduced, and also only done a few of the longhand ones. Will certainly continue on this as soon as we have a basic agreement that this style is reasonable.

…t up the Mako template (from servo:split-mako); r=nox servo/servo#10586 (comment) r? nox Source-Repo: https://github.com/servo/servo Source-Revision: 3bfa4cc7414fea760ce5c503bfbcf25262acb9d7 UltraBlame original commit: 75af6f5e3e2d5876694d389cfcaf04de2120ab6e

highfive assigned glennw Apr 13, 2016

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Apr 13, 2016

perlun reviewed Apr 13, 2016
View reviewed changes

jdm assigned SimonSapin and unassigned glennw Apr 13, 2016

highfive added the S-needs-rebase There are merge conflict errors. label Apr 14, 2016

perlun mentioned this pull request Apr 14, 2016

Extracted Mako-based code generation invokation to separate .py files. #10617

Merged

Added a TemplateLookup, so we can use %include.

fd8b550

perlun reviewed Apr 18, 2016
View reviewed changes

perlun changed the title ~~RFC: Improve properties.mako.rs file structure~~ Improve properties.mako.rs file structure Apr 19, 2016

Sorted out tidiness issues.

83716e4

perlun force-pushed the improve-mako-file-structure branch from 29ac8b1 to 83716e4 Compare April 19, 2016 05:32

SimonSapin mentioned this pull request Apr 20, 2016

Prepare related files to make it easier to split up the Mako template #10749

Merged

perlun closed this Apr 20, 2016

perlun mentioned this pull request Apr 21, 2016

Improve properties.mako.rs file structure, take 2 #10774

Merged

perlun deleted the improve-mako-file-structure branch April 22, 2016 20:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve properties.mako.rs file structure #10586

Improve properties.mako.rs file structure #10586

perlun commented Apr 13, 2016

highfive commented Apr 13, 2016

highfive commented Apr 13, 2016

perlun Apr 13, 2016

bholley Apr 13, 2016

KiChjang commented Apr 13, 2016

perlun commented Apr 13, 2016

bholley commented Apr 13, 2016

SimonSapin commented Apr 13, 2016

KiChjang commented Apr 14, 2016

perlun commented Apr 14, 2016

perlun commented Apr 14, 2016

SimonSapin commented Apr 14, 2016

bholley commented Apr 14, 2016

bors-servo commented Apr 14, 2016

perlun commented Apr 14, 2016

jdm commented Apr 14, 2016 •

edited

Loading

bholley commented Apr 14, 2016

perlun commented Apr 15, 2016

SimonSapin commented Apr 15, 2016

perlun commented Apr 18, 2016 •

edited

Loading

perlun Apr 18, 2016

SimonSapin commented Apr 18, 2016

perlun commented Apr 18, 2016

perlun commented Apr 18, 2016 •

edited

Loading

perlun commented Apr 19, 2016

perlun commented Apr 19, 2016

SimonSapin commented Apr 19, 2016

perlun commented Apr 19, 2016

perlun commented Apr 20, 2016

Improve properties.mako.rs file structure #10586

Improve properties.mako.rs file structure #10586

Conversation

perlun commented Apr 13, 2016

highfive commented Apr 13, 2016

highfive commented Apr 13, 2016

perlun Apr 13, 2016

Choose a reason for hiding this comment

bholley Apr 13, 2016

Choose a reason for hiding this comment

KiChjang commented Apr 13, 2016

perlun commented Apr 13, 2016

bholley commented Apr 13, 2016

SimonSapin commented Apr 13, 2016

KiChjang commented Apr 14, 2016

perlun commented Apr 14, 2016

perlun commented Apr 14, 2016

SimonSapin commented Apr 14, 2016

bholley commented Apr 14, 2016

bors-servo commented Apr 14, 2016

perlun commented Apr 14, 2016

jdm commented Apr 14, 2016 • edited Loading

bholley commented Apr 14, 2016

perlun commented Apr 15, 2016

SimonSapin commented Apr 15, 2016

perlun commented Apr 18, 2016 • edited Loading

perlun Apr 18, 2016

Choose a reason for hiding this comment

SimonSapin commented Apr 18, 2016

perlun commented Apr 18, 2016

perlun commented Apr 18, 2016 • edited Loading

perlun commented Apr 19, 2016

perlun commented Apr 19, 2016

SimonSapin commented Apr 19, 2016

perlun commented Apr 19, 2016

perlun commented Apr 20, 2016

jdm commented Apr 14, 2016 •

edited

Loading

perlun commented Apr 18, 2016 •

edited

Loading

perlun commented Apr 18, 2016 •

edited

Loading