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

Jump to bottom

Remove `get_*` on getters as per RFC 0344 #6224

Closed
frewsxcv opened this issue May 31, 2015 · 6 comments
Closed

Remove `get_*` on getters as per RFC 0344 #6224

frewsxcv opened this issue May 31, 2015 · 6 comments
Labels
I-refactor

Comments

@frewsxcv
Copy link
Member

@frewsxcv frewsxcv commented May 31, 2015

In an effort to:

  1. Follow the agreed upon conventions (see the RFC section below)
  2. Make our internal Servo APIs more ergonomic (shorter in this case)
  3. Assist in completing the uncompletable issue #130

I propose we remove the get_ prefix on most getters in the codebase.

If the glorious leaders of Servo approve this, I (@frewsxcv) would be willing to take on this momentous feat.

RFC

A method foo(&self) -> &T for getting the current value of the field.
A method set_foo(&self, val: T) for setting the field. (The val argument here may take &T or some other type, depending on the context.)

-- RFC 0344 § Getter/setter APIs
@Manishearth
Copy link
Member

@Manishearth Manishearth commented May 31, 2015

+1

Within the DOM we do this already, but somehow all unsafe layouty methods and bindings still have this.

Avoid touching script/ until the SM upgrade passes.

Servo and Rust don't really follow the style for Rust-y code in their internals (partially because they've been around for very long and fixing style to fix the fluid "prevailing" style is very low priority), but that doesn't mean we shouldn't try.
@Manishearth
Copy link
Member

@Manishearth Manishearth commented May 31, 2015

Actually, come to think of it I prefer having get_ on unsafe methods (very common in layout <-> DOM). When calling unsafe methods you should know what's happening, and get_ emphasizes that it's just a getter (and you know what invariants must be satisfied for the usage to be safe)
@frewsxcv
Copy link
Member Author

@frewsxcv frewsxcv commented May 31, 2015

Actually, come to think of it I prefer having get_ on unsafe methods

Just to be absolutely clear, do you believe that none of the APIs should change or should I just avoid the unsafe ones (in addition to the script/ ones)
@Manishearth
Copy link
Member

@Manishearth Manishearth commented May 31, 2015

Just avoid the unsafe ones and the script ones (unless pcwalton or someone else who uses them regularly is okay with it). Most of the unsafe ones are in script anyway.
frewsxcv added a commit to frewsxcv/servo that referenced this issue May 31, 2015
@frewsxcv
Remove `get_` prefix on getters
0ddc6df 
Part of servo#6224

I certainly didn't remove all of them; I avoided `unsafe` areas and also `components/script`
@frewsxcv frewsxcv mentioned this issue May 31, 2015
Merged
frewsxcv added a commit to frewsxcv/servo that referenced this issue Jun 2, 2015
@frewsxcv
Remove `get_` prefix on getters
435e551 
Part of servo#6224

I certainly didn't remove all of them; I avoided `unsafe` areas and also `components/script`
bors-servo pushed a commit that referenced this issue Jun 2, 2015
bors-servo
Auto merge of #6230 - frewsxcv:getters-get, r=nox
Loading status checks…
f6fe195 
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 -->
@nox nox closed this Jun 2, 2015
@nox
Copy link
Member

@nox nox commented Jun 2, 2015

Never mind.
@nox nox reopened this Jun 2, 2015
@frewsxcv frewsxcv added the I-refactor label Aug 2, 2015
@ddrmanxbxfr ddrmanxbxfr mentioned this issue Sep 8, 2015
Merged
bors-servo pushed a commit that referenced this issue Sep 11, 2015
bors-servo
Auto merge of #7559 - ddrmanxbxfr:RFC-0344-Work, r=nox
4410e8f 
Remove 'get_*' on getters as per RFC 0344 on canevas, compositing, devtools, gfx, layout, net, profile, servo and webdriver_server

Hi guys,

I just gave a big pass of RFC-0344 as per issue #6224 .

Pretty much renamed all the get_* fn that were used to fetch values. 

I hope I didn't rename too much. 

As said in the issue discussion, I didn't touch at the scripts folder so we keep the unsafe ones pretty explicit.

I've ran the whole pass of test, everything seems to be still working right :).

Please give feedback on this PR.

Thanks for looking into it.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7559)
<!-- Reviewable:end -->
bors-servo pushed a commit that referenced this issue Sep 12, 2015
bors-servo
Auto merge of #7559 - ddrmanxbxfr:RFC-0344-Work, r=nox
257ae72 
Remove 'get_*' on getters as per RFC 0344 on canevas, compositing, devtools, gfx, layout, net, profile, servo and webdriver_server

Hi guys,

I just gave a big pass of RFC-0344 as per issue #6224 .

Pretty much renamed all the get_* fn that were used to fetch values. 

I hope I didn't rename too much. 

As said in the issue discussion, I didn't touch at the scripts folder so we keep the unsafe ones pretty explicit.

I've ran the whole pass of test, everything seems to be still working right :).

Please give feedback on this PR.

Thanks for looking into it.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7559)
<!-- Reviewable:end -->
bors-servo pushed a commit that referenced this issue Sep 13, 2015
bors-servo
Auto merge of #7559 - ddrmanxbxfr:RFC-0344-Work, r=nox
Loading status checks…
b05f4aa 
Remove 'get_*' on getters as per RFC 0344 on canevas, compositing, devtools, gfx, layout, net, profile, servo and webdriver_server

Hi guys,

I just gave a big pass of RFC-0344 as per issue #6224 .

Pretty much renamed all the get_* fn that were used to fetch values. 

I hope I didn't rename too much. 

As said in the issue discussion, I didn't touch at the scripts folder so we keep the unsafe ones pretty explicit.

I've ran the whole pass of test, everything seems to be still working right :).

Please give feedback on this PR.

Thanks for looking into it.

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

@frewsxcv frewsxcv commented Sep 13, 2015

I'm pretty satisfied with the work that's been done towards this. There might be a few remaining but it's probably not worth it to keep this open
@frewsxcv frewsxcv closed this Sep 13, 2015
frewsxcv added a commit to frewsxcv/servo that referenced this issue Apr 1, 2016
@frewsxcv
Remove `get_*` on getters as per RFC 0344.
bf4db40 
https://github.com/rust-lang/rfcs/blob/master/text/0344-conventions-galore.md#gettersetter-apis

servo#6224
@frewsxcv frewsxcv mentioned this issue Apr 1, 2016
Merged
bors-servo added a commit that referenced this issue Apr 2, 2016
@bors-servo
Auto merge of #10327 - frewsxcv:get-prefix, r=ms2ger
Loading status checks…
0760e56 
Remove `get_*` on getters as per RFC 0344.

https://github.com/rust-lang/rfcs/blob/master/text/0344-conventions-galore.md#gettersetter-apis

#6224

<!-- 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/10327)
<!-- Reviewable:end -->
frewsxcv added a commit to frewsxcv/servo that referenced this issue Apr 11, 2016
@frewsxcv
Remove `get_*` on getter as per RFC 0344.
f21533a 
https://github.com/rust-lang/rfcs/blob/master/text/0344-conventions-galore.md#gettersetter-apis

servo#6224
frewsxcv added a commit to frewsxcv/servo that referenced this issue Apr 11, 2016
@frewsxcv
Remove `get_*` on getters as per RFC 0344.
11a3ce1 
https://github.com/rust-lang/rfcs/blob/master/text/0344-conventions-galore.md#gettersetter-apis

servo#6224
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Oct 1, 2019
@marco-c
servo: Merge #10327 - Remove `get_*` on getters as per RFC 0344 (from…
ff53d64 
… frewsxcv:get-prefix); r=ms2ger

https://github.com/rust-lang/rfcs/blob/master/text/0344-conventions-galore.md#gettersetter-apis

servo/servo#6224

Source-Repo: https://github.com/servo/servo
Source-Revision: 0760e56bb66e38a16543ed24385c29fd7c4a034b

UltraBlame original commit: 577d50ba257e1d66e92a68f4c80e20cd563e5363
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Oct 1, 2019
@marco-c
servo: Merge #10327 - Remove `get_*` on getters as per RFC 0344 (from…
bd1b66b 
… frewsxcv:get-prefix); r=ms2ger

https://github.com/rust-lang/rfcs/blob/master/text/0344-conventions-galore.md#gettersetter-apis

servo/servo#6224

Source-Repo: https://github.com/servo/servo
Source-Revision: 0760e56bb66e38a16543ed24385c29fd7c4a034b

UltraBlame original commit: 577d50ba257e1d66e92a68f4c80e20cd563e5363
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Oct 1, 2019
@marco-c
servo: Merge #10327 - Remove `get_*` on getters as per RFC 0344 (from…
c5acab9 
… frewsxcv:get-prefix); r=ms2ger

https://github.com/rust-lang/rfcs/blob/master/text/0344-conventions-galore.md#gettersetter-apis

servo/servo#6224

Source-Repo: https://github.com/servo/servo
Source-Revision: 0760e56bb66e38a16543ed24385c29fd7c4a034b

UltraBlame original commit: 577d50ba257e1d66e92a68f4c80e20cd563e5363
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
I-refactor
Projects
None yet
Milestone
No milestone
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
@nox @frewsxcv @Manishearth
You can’t perform that action at this time.