Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from Apr 21, 2016

Conversation

@perlun
Copy link
Contributor

perlun commented Apr 21, 2016

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 Reviewable

@highfive
Copy link

highfive commented Apr 21, 2016

Heads up! This PR modifies the following files:

  • @bholley: components/style/properties/longhand/margin.mako.rs, components/style/properties/data.py, components/style/properties/build.py, components/style/properties/longhand/padding.mako.rs, components/style/properties/longhand/position.mako.rs, components/style/properties/longhand/border.mako.rs, components/style/properties/longhand/outline.mako.rs, components/style/properties/longhand/box.mako.rs, components/style/properties/properties.mako.rs
  • @wafflespeanut: python/tidy/servo_tidy/tidy.py
@highfive
Copy link

highfive commented Apr 21, 2016

warning Warning warning

  • These commits modify style code, but no tests are modified. Please consider adding a test!
@SimonSapin
Copy link
Member

SimonSapin commented Apr 21, 2016

Reviewed 10 of 10 files at r1.
Review status: all files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.


components/style/properties/build.py, line 47 [r1] (raw file):
This shouldn’t be necessary. Isn’t properties_path the same as BASE?


components/style/properties/data.py, line 137 [r1] (raw file):
Same comment as for the to_rust_ident method below.


components/style/properties/data.py, line 161 [r1] (raw file):
This shouldn’t be necessary. Can’t these scopes use <% from data import to_rust_ident %>?


components/style/properties/properties.mako.rs, line 236 [r1] (raw file):
Rather than passing helpers=self as an argument, could these helpers be moved to yet another file that would be used through <%namespace>? http://docs.makotemplates.org/en/latest/namespaces.html


components/style/properties/properties.mako.rs, line 242 [r1] (raw file):
These comments can be removed. They don’t make much sense now that we implement a number of properties beyond CSS 2.


components/style/properties/longhand/border.mako.rs, line 5 [r1] (raw file):
Is it necessary to specify arg as an argument, or is it automatically available as part of the "context"? (In other words: please try to remove this declaration and see what happens.)


python/tidy/servo_tidy/tidy.py, line 282 [r1] (raw file):
Maybe don’t add new lines but just replace the previous one with endswith(".mako.rs") ?


Comments from Reviewable

perlun added 2 commits Apr 20, 2016
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.
@perlun perlun force-pushed the perlun:improve-mako-file-structure-v2 branch from 276ed42 to 4643737 Apr 21, 2016
@perlun
Copy link
Contributor Author

perlun commented Apr 21, 2016

Review status: 0 of 10 files reviewed at latest revision, 7 unresolved discussions.


components/style/properties/build.py, line 47 [r1] (raw file):
Thanks, good point! You're right; it is. Fixed.


components/style/properties/properties.mako.rs, line 236 [r1] (raw file):
Yep. I had to move a bunch of stuff as well (things that it depended on) but I think the end result was pretty good, so I am happy about that change.


components/style/properties/properties.mako.rs, line 242 [r1] (raw file):
Fixed.


components/style/properties/longhand/border.mako.rs, line 5 [r1] (raw file):
No, you're right; it can be omitted when you only have that as a single argument.


python/tidy/servo_tidy/tidy.py, line 282 [r1] (raw file):
Done.


components/style/properties/data.py, line 137 [r1] (raw file):
Done.


components/style/properties/data.py, line 161 [r1] (raw file):
Done.


Comments from Reviewable

@perlun
Copy link
Contributor Author

perlun commented Apr 21, 2016

@SimonSapin, as mentioned on IRC, very good feedback from you here, thanks a lot. I applied all your suggestions (in a separate commit, and rebased them both on top of the current master). Please re-check to see if we are now fine with this.

@SimonSapin
Copy link
Member

SimonSapin commented Apr 21, 2016

@bors-servo r+


Reviewed 11 of 11 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@bors-servo
Copy link
Contributor

bors-servo commented Apr 21, 2016

📌 Commit 4643737 has been approved by SimonSapin

@SimonSapin
Copy link
Member

SimonSapin commented Apr 21, 2016

@bors-servo p=10

This is blocking @bholley

@bors-servo
Copy link
Contributor

bors-servo commented Apr 21, 2016

Testing commit 4643737 with merge 69acd8e...

bors-servo added a commit that referenced this pull request Apr 21, 2016
…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.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/10774)
<!-- Reviewable:end -->
@KiChjang KiChjang assigned SimonSapin and unassigned metajack Apr 21, 2016
@edunham
Copy link
Contributor

edunham commented Apr 21, 2016

Buildbot deploy to roll out servo/saltfs#325 may affect this build, as noted on the mailing list. Sorry about the interruption!

@edunham
Copy link
Contributor

edunham commented Apr 21, 2016

@bors-servo retry

  • infra
@bors-servo
Copy link
Contributor

bors-servo commented Apr 21, 2016

@bors-servo bors-servo merged commit 4643737 into servo:master Apr 21, 2016
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@perlun perlun deleted the perlun:improve-mako-file-structure-v2 branch Apr 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.