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

Remove `get_` prefix on getters #6230

Merged
merged 1 commit into from Jun 2, 2015
Merged

Remove `get_` prefix on getters #6230

merged 1 commit into from Jun 2, 2015

Conversation

@frewsxcv
Copy link
Member

frewsxcv commented May 31, 2015

Part of #6224

I certainly didn't remove all of them; I avoided unsafe areas and also components/script

Review on Reviewable

@hoppipolla-critic-bot
Copy link

hoppipolla-critic-bot commented May 31, 2015

Critic review: https://critic.hoppipolla.co.uk/r/5138

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.

@nox
Copy link
Member

nox commented Jun 1, 2015

Review status: all files reviewed, 6 unresolved discussions, all commit checks successful.
Reviewed files:

  • components/compositing/compositor.rs @ r1
  • components/compositing/compositor_layer.rs @ r1
  • components/compositing/compositor_task.rs @ r1
  • components/devtools/actor.rs @ r1
  • components/devtools/actors/network_event.rs @ r1
  • components/devtools/actors/timeline.rs @ r1
  • components/devtools/lib.rs @ r1
  • components/gfx/font.rs @ r1
  • components/gfx/font_context.rs @ r1
  • components/gfx/paint_context.rs @ r1
  • components/gfx/paint_task.rs @ r1
  • components/gfx/platform/freetype/font.rs @ r1
  • components/gfx/platform/macos/font.rs @ r1
  • components/gfx/text/text_run.rs @ r1
  • components/layout/construct.rs @ r1
  • components/layout/fragment.rs @ r1
  • components/layout/wrapper.rs @ r1
  • components/msg/compositor_msg.rs @ r1
  • components/net/fetch/cors_cache.rs @ r1
  • components/script/clipboard_provider.rs @ r1
  • components/script/textinput.rs @ r1

components/compositing/compositor.rs, line 498 [r1] (raw file):
That's not exactly a plain old getter (it takes a PipelineId argument), should it really be renamed like this?


components/gfx/font.rs, line 44 [r1] (raw file):
Maybe use a "compute_" prefix given the implementations are quite complex and do, well, computations?


components/gfx/paint_context.rs, line 681 [r1] (raw file):
This method has many arguments, is that really a getter? Should we instead use another verb instead of just stripping the verb? There are many methods in Servo using "compute_" as a prefix, and this one looks like it could use it.


components/gfx/platform/freetype/font.rs, line 279 [r1] (raw file):
face_rec_mut() maybe?


components/layout/construct.rs, line 1478 [r1] (raw file):
construction_result_mut() maybe?


Comments from the review on Reviewable.io

@nox nox self-assigned this Jun 1, 2015
@Manishearth
Copy link
Member

Manishearth commented Jun 1, 2015

Review status: all files reviewed, 4 unresolved discussions, all commit checks successful.


components/compositing/compositor.rs, line 498 [r1] (raw file):
🚲 IMO it shouldn't be


components/gfx/paint_context.rs, line 681 [r1] (raw file):
🚲 same, compute_ preferred


components/gfx/platform/freetype/font.rs, line 279 [r1] (raw file):
🚲 👍


components/layout/construct.rs, line 1478 [r1] (raw file):
🚲 👍


Comments from the review on Reviewable.io

@nox
Copy link
Member

nox commented Jun 2, 2015

@bors-servo: r+


Review status: :shipit: all files reviewed, all discussions resolved, all commit checks successful.
Reviewed files:

  • components/compositing/compositor.rs @ r2
  • components/compositing/compositor_layer.rs @ r2
  • components/gfx/paint_context.rs @ r2
  • components/gfx/platform/freetype/font.rs @ r2
  • components/layout/construct.rs @ r2

Comments from the review on Reviewable.io

@bors-servo
Copy link
Contributor

bors-servo commented Jun 2, 2015

📌 Commit 7126d4d has been approved by nox

@nox
Copy link
Member

nox commented Jun 2, 2015

@bors-servo: r-

Sorry @frewsxcv, please squash.

@nox nox added S-needs-squash and removed S-awaiting-merge labels Jun 2, 2015
Part of #6224

I certainly didn't remove all of them; I avoided `unsafe` areas and also `components/script`
@frewsxcv frewsxcv force-pushed the frewsxcv:getters-get branch from 7126d4d to 435e551 Jun 2, 2015
@frewsxcv
Copy link
Member Author

frewsxcv commented Jun 2, 2015

It has been squashed

@nox
Copy link
Member

nox commented Jun 2, 2015

@bors-servo: r+

-S-awaiting-review -S-needs-squash +S-awaiting-merge


Review status: all files reviewed, all discussions resolved, some commit checks pending.


Comments from the review on Reviewable.io

@bors-servo
Copy link
Contributor

bors-servo commented Jun 2, 2015

📌 Commit 435e551 has been approved by nox

@bors-servo
Copy link
Contributor

bors-servo commented Jun 2, 2015

Testing commit 435e551 with merge f6fe195...

bors-servo pushed a commit that referenced this pull request Jun 2, 2015
Part of #6224

I certainly didn't remove all of them; I avoided `unsafe` areas and also `components/script`

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/6230)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jun 2, 2015

☀️ Test successful - android, gonk, linux1, linux2, mac1, mac2

@bors-servo bors-servo merged commit 435e551 into servo:master Jun 2, 2015
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@frewsxcv frewsxcv deleted the frewsxcv:getters-get branch Oct 9, 2015
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

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