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

Clean up orphan functions and tiny one-off helpers #6022

Open
wants to merge 46 commits into
base: main
Choose a base branch
from
Open

Clean up orphan functions and tiny one-off helpers #6022

wants to merge 46 commits into from

Conversation

teunbrand
Copy link
Collaborator

This PR doesn't adress an issue, but I think it is worth to consider anyway.

In essence, it tries to cleanup three types of functions:

  1. Orphan functions that were used at some point, but have fallen in disuse because surrounding code has been refactored.
  2. Tiny helper functions that are used only once and can easily be used written out in full.
  3. Mirror functions where two versions of nearly identical code existed, like the simplify()/simplify_formula() or as.quoted()/as_quoted().

@teunbrand
replace single use internal functions
20fa50f
@teunbrand
move helper to location where it is used
0bfbcc3
@teunbrand
remove orphan functions
79866e8
@teunbrand
replace and remove redundant adjust_breaks() function
a5a1ce5
@teunbrand
replace df_rows() with vec_slice()
c4c61de
@teunbrand
inline f_as_facets_list()
2ce92fb
@teunbrand
inline find_origin()
15241a6
@teunbrand
inline firstUpper()
2e9b8c2
@teunbrand
replace has_name()
0cca7a3
@teunbrand
replace interleave() with vec_interleave()
5f4a4b9
@teunbrand
remove cunion()
79fa0b1
@teunbrand
inline is_dotted_var()
abc62dd
@teunbrand
inline is_facets()
4a3ffff
@teunbrand
inline is_labeller()
9976913
@teunbrand
inline is_missing_arg()
b4e0d80
@teunbrand
replace is_npc() (we partially backport unitType())
be4081a
@teunbrand
inline is_scalar_numeric()
70cc78b
@teunbrand
inline is.margin()
997d9fe
@teunbrand
inline is.sec_axis()
1bdc20d
@teunbrand
inline is.subclass()
b044fa2
@teunbrand
inline is_triple_bang()
490c9c3
@teunbrand
remove justify_grobs()
4ebdea3
@teunbrand
inline label_variable()
e086817
@teunbrand
more responsibility for parse_axes_labeling(), so that it is less d…
024a1b6 
…istracting
@teunbrand
remove resolve_guide()
b2de86e
@teunbrand
inline revalue
f7c8cb6
@teunbrand
simplify scale_flip_position()
3232471
@teunbrand
remove as.quoted() (note there is still as_quoted())
37da401
@teunbrand
replace simplify_formula() with simplify()
6ac62cf
@teunbrand
inline single_value()
db6feb8
@teunbrand teunbrand mentioned this pull request Aug 2, 2024
Open
thomasp85
thomasp85 reviewed Aug 23, 2024
View reviewed changes
Copy link
Member

@thomasp85 thomasp85 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bunch of comments since this is a large PR. Overall good though I have one main objection as you can see in the comments

R/aes-evaluation.R Show resolved Hide resolved
R/axis-secondary.R Outdated Show resolved Hide resolved
R/bin.R Outdated Show resolved Hide resolved
R/coord-sf.R Outdated Show resolved Hide resolved
R/grob-dotstack.R Show resolved Hide resolved
R/labeller.R Outdated Show resolved Hide resolved
R/layer.R Show resolved Hide resolved
R/margins.R Show resolved Hide resolved
R/plot-construction.R Outdated Show resolved Hide resolved
R/utilities.R Show resolved Hide resolved
@teunbrand
Copy link
Collaborator Author

I'm not against all these class tests like is.margin(), but let's discuss whether we should be consistent about exporting them.
If it is only for internal use, and they are used only once, then I think inherits() prevents some clutter.

@teunbrand

This comment was marked as resolved.

Sign in to view
@teunbrand
Copy link
Collaborator Author

I restored the class tests. For user-facing classes, I've collected all the tests in one place and exported them.

teunbrand
teunbrand commented Aug 27, 2024
View reviewed changes
R/utilities-checks.R Outdated Show resolved Hide resolved
thomasp85
thomasp85 reviewed Sep 13, 2024
View reviewed changes
R/utilities-checks.R Outdated Show resolved Hide resolved
@thomasp85
Copy link
Member

I think the testers needs to be below the class definition, not in their own file. This is our general MO

@teunbrand
abolish uniquecols()'s rownames
13b8d36
@teunbrand
move is.*() functions to class definitions
d7092aa
@teunbrand
resolve merge conflict
16b35e9 
Merge branch 'main' into orphan_functions

# Conflicts:
#	tests/testthat/_snaps/utilities.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thomasp85 thomasp85 thomasp85 left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@teunbrand @thomasp85