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.

Sign up for GitHub

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

Standardise getters, setters and updaters #5568

Closed
teunbrand opened this issue Dec 6, 2023 · 7 comments · Fixed by #5802
Closed

Standardise getters, setters and updaters #5568

teunbrand opened this issue Dec 6, 2023 · 7 comments · Fixed by #5802
Labels
internals 🔎

Comments

@teunbrand
Copy link
Collaborator

teunbrand commented Dec 6, 2023 •
edited

There are various functions that get or set things, but the names aren't standardised.
Some example getter functions are:

layer_data()
layer_grob()
layer_scales() # these aren't really related to layers?
theme_get()
last_plot()
get_alt_text()

Some example setter functions are:

theme_set()
set_last_plot()

Some example of updater functions are:

update_geom_defaults()
update_stat_defaults()
theme_update()
add_ggplot()/ggplot_add()

The non-standardisation of these functions confuse the API somewhat. For example theme_bw() is a theme in its own right but theme_get() is an interface to the global theme. Likewise, layer_sf() is a specialty layer, whereas layer_data() is not a layer.

I'd like to propose that these type of functions always use a get_, set_ or update_ prefixes so that it is easy to see what getters, setters and updaters are available through tab-completion.

For example we'd have:

get_layer_data()
get_layer_grob()
get_scales()
get_theme()
get_last_plot()
get_alt_text() # identical
set_theme()
set_last_plot() # identical
update_theme()
teunbrand added a commit to teunbrand/ggplot2 that referenced this issue Dec 14, 2023
@teunbrand
rename getter (see tidyverse#5568)
65e3e5f
teunbrand added a commit that referenced this issue Dec 15, 2023
@teunbrand
Extracting guide data (version 3) (#5506)
24dfd6e 
* add `guide_data()` function

* add tests

* document

* rename getter (see #5568)

* swap from location-based to panel-based
@davidhodge931
Copy link

+1 for this. The consistency proposed here would really help. Don't suppose this could be in 3.5.1?

@davidhodge931
Copy link

davidhodge931 commented Mar 23, 2024 •
edited

I also find the prefix update a bit long, which makes update_geom_defaults() hard to read.

Perhaps add_* could be used here instead of update_*?

So:
add_geom_defaults()
add_theme()

Instead of:
update_geom_defaults()
update_theme()

Maybe it's slightly less precise than update, but I feel it's close enough and much more readable

@teunbrand
Copy link
Collaborator Author

The code currently uses 'add' as unofficial indicator that it would work by using + during plot construction in ggplot_add(), add_ggplot() and add_theme(). My preference goes to the get/set syntax for brevity and symmetry reasons.

@davidhodge931
Copy link

Oh okay, so have set_geom_defaults instead of update_geom_defaults? I think that'd be way better. I like brevity/symmetry too

@teunbrand
Copy link
Collaborator Author

The expectation for a set_geom_defaults() would be that you have to provide the complete set of default aesthetics, whereas for update_geom_defaults() it is expected you can provide a subset of aesthetics with the rest remaining as-is. That'd be analogous to how the theme_set() vs theme_update() mechanics work.

@teunbrand teunbrand mentioned this issue Mar 25, 2024
Merged
@davidhodge931
Copy link

Assume this isn't planned to be implemented for 3.5.1 @teunbrand ?

@teunbrand
Copy link
Collaborator Author

teunbrand commented Mar 27, 2024 •
edited

I wouldn't count on it. It would probably be fine to include in 3.5.1 as it is backward compatible, but priorities for PR review are fixing regressions, safe bugfixes and doc updates. You can take a look at the milestones to see what is planned (and infer what isn't).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
internals 🔎
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Standardise get_*, set_*, update_* and replace_* prefixes. teunbrand/ggplot2
2 participants
@teunbrand @davidhodge931