Skip to content

/ servo

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

Automatically verify that derive() lists are alphabetically ordered #… #18179

Merged
merged 2 commits into from Aug 23, 2017
Merged

Automatically verify that derive() lists are alphabetically ordered #… #18179

merged 2 commits into from Aug 23, 2017
+568 −552

Conversation

@davidcl
Copy link
Contributor

davidcl commented Aug 21, 2017 •
edited by SimonSapin

Automatically verify that derive() lists are alphabetically ordered #18172

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #18172 (github issue number if applicable).
  • There are tests for these changes OR
  • These changes do not require tests because _____

This change is Reviewable
@highfive
Copy link

highfive commented Aug 21, 2017

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @wafflespeanut (or someone else) soon.
@highfive
Copy link

highfive commented Aug 21, 2017

Heads up! This PR modifies the following files:

  • @wafflespeanut: python/tidy/servo_tidy_tests/test_tidy.py, python/tidy/servo_tidy_tests/rust_tidy.rs, python/tidy/servo_tidy/tidy.py
  • @edunham: python/tidy/servo_tidy_tests/test_tidy.py, python/tidy/servo_tidy_tests/rust_tidy.rs, python/tidy/servo_tidy/tidy.py
@KiChjang
KiChjang reviewed Aug 21, 2017
View changes
python/tidy/servo_tidy/tidy.py Outdated
@@ -493,6 +493,18 @@ def check_rust(file_name, lines):
line = merged_lines + line
merged_lines = ''

# derive traits should be alphabetically ordered
if line.startswith("#[derive"):

This comment has been minimized.

Sign in to view
@KiChjang

KiChjang Aug 21, 2017

Member

Does startswith ignore initial whitespaces?

This comment has been minimized.

Sign in to view
@davidcl

davidcl Aug 22, 2017

Author Contributor

no, I will fix that

This comment has been minimized.

Sign in to view
@davidcl

davidcl Aug 22, 2017

Author Contributor

Done on b332239
@jdm
Copy link
Member

jdm commented Aug 21, 2017

Wow, we have a lot of prior violations of this rule.
@davidcl davidcl closed this Aug 22, 2017
@davidcl
Copy link
Contributor Author

davidcl commented Aug 22, 2017

@jdm test-tidy does not report anything on my machine, what ./mach command are you using ?

./mach test-tidy 
Checking the config file...
Checking directories for correct file extensions...
Checking files for tidiness...

tidy reported no errors.
@jdm
Copy link
Member

jdm commented Aug 22, 2017

@davidcl ./mach test-tidy --all. By default test-tidy only looks at files that were changed in your local commits.
@jdm jdm reopened this Aug 22, 2017
@davidcl
Copy link
Contributor Author

davidcl commented Aug 22, 2017 •
edited

If the tidy derive is fully agreed by other contributors, I could also fix the existing codebase accordingly.

The tidy derive should also handle #[derive(Copy,Debug $(, $derived_trait )* ))] syntax, to me the macro expansion should always be at the end ; could you confirm ? done
@jdm
Copy link
Member

jdm commented Aug 22, 2017

CC @nox
@nox
Copy link
Member

nox commented Aug 22, 2017

Nice!
@jdm
jdm reviewed Aug 22, 2017
View changes
components/style/gecko/generated/structs_debug.rs Outdated
const _ as usize } , 32usize , concat ! (
"Alignment of field: " , stringify ! ( nsAutoString ) ,
"::" , stringify ! ( mStorage ) ));
pub type nsAutoStringN_self_type = u8;

This comment has been minimized.

Sign in to view
@jdm

jdm Aug 22, 2017

Member

Ooh, we should exclude this directory from these changes. This is generated code that's out of our control.

This comment has been minimized.

Sign in to view
@davidcl

davidcl Aug 23, 2017

Author Contributor

Done on e6a7943
@KiChjang
Copy link
Member

KiChjang commented Aug 22, 2017

Also, merge commits are not accepted. Please rebase your PR against the master branch instead.
@davidcl davidcl force-pushed the davidcl:master branch from 3138309 to e6a7943 Aug 23, 2017
@davidcl
Copy link
Contributor Author

davidcl commented Aug 23, 2017

@KiChjang should I also prefer rebase --squash for 2f15ae2 e99c696 and 3a959b7 ? as I rewrote git history it should be cleaner to only have the tidy derive in one commit and rust code format on another.
@jdm
Copy link
Member

jdm commented Aug 23, 2017

There's a new tidy failure from rebasing, and this as well:

error[E0428]: a type named `RuleInclusion` has already been defined in this module

   --> /home/travis/build/servo/servo/components/style/stylist.rs:474:1

    |

81  | / pub enum RuleInclusion {

82  | |     /// Include rules for style sheets at all cascade levels.  This is the

83  | |     /// normal rule inclusion mode.

84  | |     All,

...   |

87  | |     DefaultOnly,

88  | | }

    | |_- previous definition of `RuleInclusion` here

...

474 | / pub enum RuleInclusion {

475 | |     /// Include rules for style sheets at all cascade levels.  This is the

476 | |     /// normal rule inclusion mode.

477 | |     All,

...   |

480 | |     DefaultOnly,

481 | | }

    | |_^ `RuleInclusion` already defined
@jdm
Copy link
Member

jdm commented Aug 23, 2017

Rather, travis reports a bunch of errors that look like incorrect rebasing.
@jdm
jdm approved these changes Aug 23, 2017
View changes
Copy link
Member

jdm left a comment

This looks great! Let's squash the three commits you mentioned together, too.
components/style/stylist.rs Outdated
@@ -76,9 +76,85 @@ struct DocumentCascadeData {
precomputed_pseudo_element_decls: PerPseudoElementMap<Vec<ApplicableDeclarationBlock>>,
}

impl DocumentCascadeData {

This comment has been minimized.

Sign in to view
@jdm

jdm Aug 23, 2017

Member

Here's the rebase issue.

This comment has been minimized.

Sign in to view
@davidcl

davidcl Aug 23, 2017

Author Contributor

Thanks, I re-run my upgrade script after the rebase (rather than before) and everything is fine now.
@jdm jdm assigned jdm and unassigned wafflespeanut Aug 23, 2017
@jdm
Copy link
Member

jdm commented Aug 23, 2017 

./components/style/gecko/generated/pseudo_element_definition.rs:6: derivable traits list is not in alphabetical order

	expected: Clone, Debug, Eq, Hash, PartialEq

	found: Clone, Debug, PartialEq, Eq, Hash
@jdm jdm added the S-fails-tidy label Aug 23, 2017
davidcl added 2 commits Aug 21, 2017
@davidcl
Automatically verify that derive() lists are alphabetically ordered
ab73f3d
@davidcl
order derivable traits lists
Loading status checks…
c5fe235 
Ignoring :
 - **generated**.rs
 - python/tidy/servo_tidy_tests/rust_tidy.rs
@davidcl davidcl force-pushed the davidcl:master branch from e6a7943 to c5fe235 Aug 23, 2017
@jdm
Copy link
Member

jdm commented Aug 23, 2017

@bors-servo: r+
Thanks!
@bors-servo
Copy link
Contributor

bors-servo commented Aug 23, 2017

📌 Commit c5fe235 has been approved by jdm
@jdm jdm removed the S-fails-tidy label Aug 23, 2017
@bors-servo
Copy link
Contributor

bors-servo commented Aug 23, 2017

⌛ Testing commit c5fe235 with merge 3892d4e6ff260e0a98caa4238b7dd4f0fd6a93c3...
@bors-servo
Copy link
Contributor

bors-servo commented Aug 23, 2017

💔 Test failed - mac-rel-wpt3
@jdm
Copy link
Member

jdm commented Aug 23, 2017

@bors-servo: retry

  • git failure
@bors-servo
Copy link
Contributor

bors-servo commented Aug 23, 2017

⌛ Testing commit c5fe235 with merge 4743696...
bors-servo added a commit that referenced this pull request Aug 23, 2017
@bors-servo
Auto merge of #18179 - davidcl:master, r=jdm
Loading status checks…
4743696 
Automatically verify that derive() lists are alphabetically ordered #…

<!-- Please describe your changes on the following line: -->
Automatically verify that derive() lists are alphabetically ordered #18172

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #18172 (github issue number if applicable).

<!-- Either: -->
- [X] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18179)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Aug 23, 2017

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: jdm
Pushing 4743696 to master...
@bors-servo bors-servo merged commit c5fe235 into servo:master Aug 23, 2017
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
This was referenced Aug 23, 2017
Closed
Closed
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KiChjang KiChjang

@jdm jdm

Assignees

@jdm jdm

Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

Automatically verify that derive() lists are alphabetically ordered
7 participants
@davidcl @highfive @jdm @nox @KiChjang @bors-servo @wafflespeanut
You can’t perform that action at this time.