Skip to content

Add vignette with comparison tables for base R vs fs#168

Merged
jimhester merged 7 commits intor-lib:masterfrom
xvrdm:master
Feb 19, 2019
Merged

Add vignette with comparison tables for base R vs fs#168
jimhester merged 7 commits intor-lib:masterfrom
xvrdm:master

Conversation

@xvrdm
Copy link
Copy Markdown
Contributor

@xvrdm xvrdm commented Jan 28, 2019

This PR contains a vignette with comparison tables for base R vs fs.

As @cderv mentioned on #156, there are probably more details that would be relevant for functions like fs_path().

Since this is the first vignette for this package (that's my understanding...), it modifies also a bit the DESCRIPTION and .gitignore. I tried to keep only what seems standard on other packages but not sure if I did well.

@jimhester
Copy link
Copy Markdown
Member

FWIW usethis::use_vignette() is what I would recommend to use when setting up vignettes, it will setup everything needed for you.

@xvrdm
Copy link
Copy Markdown
Contributor Author

xvrdm commented Jan 28, 2019

This is what I used actually. Does the output looks weird?

Copy link
Copy Markdown
Member

@jimhester jimhester left a comment

Choose a reason for hiding this comment

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

Thanks this is looking good!

I think we should put fs as the first column in the table rather than base, so it woudl be fs, base, shell.

Comment thread vignettes/base-vs-fs.Rmd Outdated

| base | fs | shell |
|-------------------------------------+---------------------------------------------+------------------------------------|
| *No easy way* | `file_chmod("/path", "mode")` | `chmod mode /path` |
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This should be Sys.chmod()

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah could not find this one!
What is a good PR process: if I added this in the last commit, do I click "Resolve conversation" or do I leave it to the Reviewer?

Comment thread vignettes/base-vs-fs.Rmd Outdated
| base | fs | shell |
|--------------------------------------------------+------------------------------------------------------+-------------------------------|
| `file.path("top_dir", "nested_dir", "file.ext")` | `path("top_dir", "nested_dir", "file", ext = "ext")` | `top_dir/nested_dir/file.ext` |
| `path.expand()` | `path_expand("~/path")` | *Need scripting.* |
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

maybe the shell one could be realpath -m -s ~/path. realpath is a GNU specific thing, but seems reasonable to use here.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It would also be nice to include path_real() and path_norm() as additional entries here, but I can do that later if needed.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added realpath -m -s ~/path (could not try it on this machine).
Maybe it is better that you add path_real() and path_norm(). I am not sure what would be the fs and shell short equivalents.

Add realname as a shell equivalent to fs::path_expand
Add Sys.chmod as base equivalent to fs::file_chmod
@jimhester
Copy link
Copy Markdown
Member

Thanks,
Can you please add a bullet to NEWS? It should briefly describe the change and end with (@yourname, #issuenumber).

Then I will merge this and can flesh out anything remaining (like path_real() and path_norm()) at a later date.

@xvrdm
Copy link
Copy Markdown
Contributor Author

xvrdm commented Feb 19, 2019

Ok thanks, I added a line to NEWS.

I pushed the change but not sure if I shall have synced my fork first as I am now a few commits behind the original (sorry first time contribution...).

@jimhester
Copy link
Copy Markdown
Member

Thanks for your work on this, looks great!

@jimhester
Copy link
Copy Markdown
Member

And thank you for coming to the tidyverse developer day!

@jimhester jimhester merged commit 9d39853 into r-lib:master Feb 19, 2019
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.

2 participants