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

Optionally print project path in table notes annotation #341

Merged
merged 19 commits into from
Jun 21, 2024
Merged

Conversation

kylebaron
Copy link
Contributor

@kylebaron kylebaron commented Jun 20, 2024

Summary

  • Retain the current behavior of writing the name of the output file into the table annotation
  • As noted in the attached vignette, the big change here is ability to add both the output file name and output path (directory) into the table annotation; this means we need to lock in the final location for the table as we build the table; the vignette shows this.
  • tab_notes() gains the following arguments
    • output_dir where (what directory) to save the file; respects pmtables.dir
    • path.type how to render output_dir in the annotation;
    • See implementation details
  • New function: format_table_path(); does equivalent of what we already do in mrggsave

Current default

Currently, we can take in the output file name and that gets
included in a table note. However, the decision about where
to save that file is deferred until we actually call
stable_save().

The changes to pmtables include accepting the output file path
when we create the table. Now, if the user passes the output path
and it is included in the table annotation, we have to respect that
information when saving; it can't be changed after the annotation
is created.

Implementation detail

Currently, it is the tab_notes() function that handles all the
arguments for R source file, output file, output directory etc. But
this is a but of a misnomer now, because the function doing more
than just writing a note and saving the file name for later; real
decisions about output file locations are being made.

I have added a non-exported function tab_files() that handles
all the file / path manipulation and arguments that get passed
to tab_notes() eventually go to tab_files(). In the future,
I will open up tab_files() since that more accurately reflects
where the file / path proecessing is taking place.

source-path.pdf

@kylebaron kylebaron marked this pull request as draft June 20, 2024 15:58
R/utils.R Outdated Show resolved Hide resolved
R/utils.R Outdated Show resolved Hide resolved
R/table-notes.R Outdated Show resolved Hide resolved
@kyleam
Copy link
Contributor

kyleam commented Jun 21, 2024

I'll interactively test this a bit, but this is looking good to me aside from my minor comments above.

Copy link
Collaborator

@KatherineKayMRG KatherineKayMRG left a comment

Choose a reason for hiding this comment

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

From a user perspective, this worked well for me on the project and I like the opt in approach. I'll leave the more thorough code review to @kyleam

Copy link
Contributor

@kyleam kyleam left a comment

Choose a reason for hiding this comment

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

LGTM aside from typos mentioned above

@kylebaron
Copy link
Contributor Author

Thanks, @kyleam ; working on the typos now.

@kylebaron kylebaron marked this pull request as ready for review June 21, 2024 15:50
@kylebaron kylebaron merged commit 3504917 into main Jun 21, 2024
5 checks passed
@kylebaron kylebaron deleted the source-path branch June 21, 2024 15:57
@kylebaron kylebaron mentioned this pull request Jun 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants