Skip to content

Permit function devices to use file argument #3810

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
Merged
merged 4 commits into from
Feb 24, 2020
Merged

Permit function devices to use file argument #3810

merged 4 commits into from
Feb 24, 2020

Conversation

bwiernik
Copy link
Contributor

@bwiernik bwiernik commented Feb 7, 2020

Devices like devEMF::emf or Cairo::CairoSVG use file instead of filename. They currently fail when provided in device to ggsave. This fixes that.

Closes #3807

yutannihilation
yutannihilation reviewed Feb 21, 2020
View reviewed changes
Copy link
Member

@yutannihilation yutannihilation left a comment

Choose a reason for hiding this comment

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

Thanks for catching this! This basically looks good to me. One corner case in my mind is when a function has both file and filename in its arguments, but I expect such functions are very few (or none).

Could you add a NEWS bullet?

bwiernik and others added 2 commits February 21, 2020 12:27
@bwiernik @yutannihilation
Put return() in {} block following tidystyle
b9b3aa0 
Co-Authored-By: Hiroaki Yutani <yutani.ini@gmail.com>
@bwiernik
Update NEWS
354b455
@bwiernik
Copy link
Contributor Author

Okay, fixed the styling and added a NEWS bullet as requested.

yutannihilation
yutannihilation reviewed Feb 24, 2020
View reviewed changes
NEWS Outdated
Comment on lines 9 to 11
* Support graphics devices that use the `file` argument instead of `fileneame`
in `ggsave()` (@bwiernik, 3810)

Copy link
Member

Choose a reason for hiding this comment

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

Ah, sorry, we are now using NEWS.md instead of NEWS. Could you move this to NEWS.md?

@yutannihilation yutannihilation merged commit 53edc51 into tidyverse:master Feb 24, 2020
@yutannihilation
Copy link
Member

Thanks!

@bwiernik bwiernik deleted the ggsave-file-arg branch February 25, 2020 00:34
sthagen added a commit to sthagen/tidyverse-ggplot2 that referenced this pull request Feb 25, 2020
@sthagen
Merge pull request #4 from tidyverse/master
3fb1d12 
Permit function devices to use `file` argument (tidyverse#3810)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yutannihilation yutannihilation yutannihilation approved these changes

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

Successfully merging this pull request may close these issues.

ggsave() can't be used with devices taking file as argument instead of filename
2 participants
@bwiernik @yutannihilation