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

Run rustfmt on selectors, servo_arc, and style #20603

Merged
merged 3 commits into from Apr 11, 2018

Conversation

Projects
None yet
8 participants
@bholley
Copy link
Contributor

commented Apr 10, 2018

Now that rustfmt is getting close to stable, and work on the style system has died down a bit, it seemed like an opportune time to auto-format the style crates.

The first commit disables import reordering, since tidy and rustfmt don't currently agree on the correct ordering. The second commit does a bunch of manual fixups such that the output of rustfmt passes tidy. The third commit runs rustfmt on the three aforementioned crate.

There are a few dozen warnings in the style crate about lines longer than 100 characters. It would be good to fix these, but I don't have time for that now.


This change is Reviewable

@highfive

This comment has been minimized.

Copy link

commented Apr 10, 2018

Heads up! This PR modifies the following files:

  • @canaltinova: components/style/values/computed/pointing.rs, components/style/values/generics/flex.rs, components/style/stylesheets/style_rule.rs, components/style/gecko_bindings/sugar/ns_css_value.rs, components/style/invalidation/element/invalidator.rs and 166 more
  • @emilio: components/style/values/computed/pointing.rs, components/style/values/generics/flex.rs, components/style/stylesheets/style_rule.rs, components/style/gecko_bindings/sugar/ns_css_value.rs, components/style/invalidation/element/invalidator.rs and 166 more
@highfive

This comment has been minimized.

Copy link

commented Apr 10, 2018

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
  • These commits modify style code, but no tests are modified. Please consider adding a test!
@bholley

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2018

@bholley

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2018

(and @heycam of course)

r? @SimonSapin

@highfive highfive assigned SimonSapin and unassigned glennw Apr 10, 2018

@bholley

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2018

CC @Manishearth also

@upsuper

This comment has been minimized.

Copy link
Member

commented Apr 10, 2018

I wonder is this really just running rustfmt on the crates? It seems to be doing more things than I would thought a formatter normally does, e.g. removing use for those only used once.

Also I find it a bit hard to follow how some decision is made for formatting, for example, I see conversion from single line if ... { A } else { B } to multi-line, and vice versa.

@upsuper

This comment has been minimized.

Copy link
Member

commented Apr 10, 2018

Hmmm, actually not. Many of the changes I wouldn't expect formatter to do are from the manual fixup. That explains :)

)
})?
)
let r = input.parse_nested_block(|input| {

This comment has been minimized.

Copy link
@emilio

emilio Apr 10, 2018

Member

what does r mean? Maybe just selector?

@emilio

This comment has been minimized.

Copy link
Member

commented Apr 10, 2018

CI is also red:

error: unused import: `malloc_size_of::MallocUnconditionalShallowSizeOf`
  --> components/style/stylesheets/style_rule.rs:10:5
   |
10 | use malloc_size_of::MallocUnconditionalShallowSizeOf;
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: `-D unused-imports` implied by `-D warnings`
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2018

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

@SimonSapin

This comment has been minimized.

Copy link
Member

commented Apr 10, 2018

@bholley Could you include in the PR message (which will go in the merge commit) what version of rustfmt you used and what command you ran to produce this commit?

@bholley bholley force-pushed the bholley:rustfmt_style branch from 3f08166 to e55b414 Apr 10, 2018

@bholley

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2018

Addressed review comments. If someone could stamp this soon I'd appreciate it, since it bitrots quickly. :-)

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2018

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

@bholley bholley force-pushed the bholley:rustfmt_style branch from e55b414 to b3d39c4 Apr 10, 2018

@Manishearth
Copy link
Member

left a comment

r=me

@bholley

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2018

@bors-servo r=Manishearth

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2018

📌 Commit b3d39c4 has been approved by Manishearth

@emilio

emilio approved these changes Apr 10, 2018

Copy link
Member

left a comment

There are a few of these that look like regressions to me. Maybe @nrc may take a look and see if there's something unexpected in some of these?

Also, is there any plan / way to enforce its usage? My plan to start doing this, btw, was enforcing usage of rustfmt-format-diff (https://github.com/rust-lang-nursery/rustfmt/blob/master/src/format-diff/main.rs) on incoming commits. But not sure if there's any better idea around.

Anyway, as much as I dislike some particular changes, consistency is king I guess...

.any(|part| case.eq(part.as_bytes(), s))
}
},
AttrSelectorOperator::Includes => element_attr_value

This comment has been minimized.

Copy link
@emilio

emilio Apr 10, 2018

Member

Weird that this didn't include braces.

Component::Slotted(..) |
Component::PseudoElement(..) |
Component::LocalName(..) => {
Component::Slotted(..) | Component::PseudoElement(..) | Component::LocalName(..) => {

This comment has been minimized.

Copy link
@emilio

emilio Apr 10, 2018

Member

This doesn't look better, honestly, but I guess it's fine.

@@ -52,8 +52,8 @@ impl ElementSelectorFlags {
/// Returns the subset of flags that apply to the parent.
pub fn for_parent(self) -> ElementSelectorFlags {
self & (ElementSelectorFlags::HAS_SLOW_SELECTOR |
ElementSelectorFlags::HAS_SLOW_SELECTOR_LATER_SIBLINGS |

This comment has been minimized.

Copy link
@emilio

emilio Apr 10, 2018

Member

This also looks spooky.

matches!(
selector.combinator_at_parse_order(from_offset),
Combinator::SlotAssignment | Combinator::PseudoElement
)),

This comment has been minimized.

Copy link
@emilio

emilio Apr 10, 2018

Member

Don't understand how this makes more sense than what we had either but.. I guess.

},
_ => true,
}
let all_match = selector_iter.clone().all(|simple| match *simple {

This comment has been minimized.

Copy link
@emilio

emilio Apr 10, 2018

Member

Ugh, this one looks really bad :(

@@ -149,7 +156,7 @@ impl<'a, 'b, 'i> AtRuleParser<'i> for CounterStyleRuleParser<'a, 'b> {

macro_rules! checker {
($self:ident._($value:ident)) => {};
($self:ident.$checker:ident($value:ident)) => {
($self:ident. $checker:ident($value:ident)) => {

This comment has been minimized.

Copy link
@emilio

emilio Apr 10, 2018

Member

This is touching a macro, and I'm not sure it's fine, actually... I guess it is but it looks fishy at best.

+ Clone
+ SelectorsElement<Impl = SelectorImpl>
pub trait TElement:
Eq + PartialEq + Debug + Hash + Sized + Copy + Clone + SelectorsElement<Impl = SelectorImpl>

This comment has been minimized.

Copy link
@emilio

emilio Apr 10, 2018

Member

This looks also like a regression to me, but...

write!(f, "Invalid @counter-style rule: 'system: extends …' with 'additive-symbols'")
}
},
ContextualParseError::InvalidCounterStyleWithoutSymbols(ref system) => write!(

This comment has been minimized.

Copy link
@emilio

emilio Apr 10, 2018

Member

These look way worse too.

#[derive(Clone, Copy, Debug, Eq, MallocSizeOf, Ord)]
#[derive(PartialEq, PartialOrd, ToComputedValue, ToCss)]
#[derive(Clone, Copy, Debug, Eq, MallocSizeOf, Ord, PartialEq, PartialOrd, ToComputedValue,
ToCss)]

This comment has been minimized.

Copy link
@emilio

emilio Apr 10, 2018

Member

It also merges #[derive(..)] lines? Is there a way to prevent it from doing that?

)
where
Impl: SelectorImpl
) where

This comment has been minimized.

Copy link
@emilio

emilio Apr 10, 2018

Member

This () where) looks weird, is it expected?

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2018

🔒 Merge conflict

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2018

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

bholley added some commits Apr 10, 2018

Disable import reordering for now.
Tidy wants it one way, and the latest nightly of rustfmt wants it
another way.
Run rustfmt on selectors, servo_arc, and style.
This was generated with:

./mach cargo fmt --package selectors &&
./mach cargo fmt --package servo_arc &&
./mach cargo fmt --package style

Using rustfmt 0.4.1-nightly (a4462d1 2018-03-26)
@emilio

This comment has been minimized.

Copy link
Member

commented Apr 11, 2018

FWIW I think something like #20617 would be less invasive. Arguably needs more polish and what not. But massive reformats pollute the history quite heavily, it's better to do it incrementally if possible IMO.

@bholley bholley force-pushed the bholley:rustfmt_style branch from b3d39c4 to c99bcdd Apr 11, 2018

@bholley

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2018

The downsides I see to per-patch reformatting are:
(1) Patches that should otherwise be small (i.e. renames) can suddenly get big as they reformat surrounding code, making them harder to review.
(2) It's harder for new contributors to infer the correct style from surrounding code, because it may or may not be consistent with the canonical style.

There's definitely room for disagreement here. But given that most people have expressed approval for doing it this way, and given that I've already done the work here, I'm inclined to land it.

@bholley

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2018

@bors-servo r=Manishearth p=5

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 11, 2018

📌 Commit c99bcdd has been approved by Manishearth

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 11, 2018

⌛️ Testing commit c99bcdd with merge 9a900ef...

bors-servo added a commit that referenced this pull request Apr 11, 2018

Auto merge of #20603 - bholley:rustfmt_style, r=Manishearth
Run rustfmt on selectors, servo_arc, and style

Now that rustfmt is getting close to stable, and work on the style system has died down a bit, it seemed like an opportune time to auto-format the style crates.

The first commit disables import reordering, since tidy and rustfmt don't currently agree on the correct ordering. The second commit does a bunch of manual fixups such that the output of rustfmt passes tidy. The third commit runs rustfmt on the three aforementioned crate.

There are a few dozen warnings in the style crate about lines longer than 100 characters. It would be good to fix these, but I don't have time for that now.

<!-- 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/20603)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 11, 2018

@bors-servo bors-servo merged commit c99bcdd into servo:master Apr 11, 2018

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.