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

Tweak "From base R" vignette: #483

Merged
merged 8 commits into from
Dec 7, 2022
Merged

Tweak "From base R" vignette: #483

merged 8 commits into from
Dec 7, 2022

Conversation

salim-b
Copy link
Contributor

@salim-b salim-b commented Dec 6, 2022

I think the new base R <-> stringr comparison table is incredibly useful! Just want to improve it in the following ways:

  • Define data in tribble and generate MD table from it.
  • The new table is ordered alphabetically by stringr function name for now, which I think is more accessible (base R API is just a mess whether we sort it alphabetically by name or not). But order can easily be changed later on if desired.
  • The new table has links to stringr's pkgdown reference when built by pkgdown thanks to downlit::autolink_url(). Removed as per @hadley's request.
  • Add missing str_extract_all() base R equivalent
  • Fixed fn param names in table.
  • Fix typo in fn name.
  • Use comment-style in-body YAML chunk opts
  • Cosmetic changes

- Define data in tribble and generate MD table from it.
- The new table is ordered alphabetically by *stringr function name* for now, which I think is more accessible (base R API is just a mess whether we sort it alphabetically by name or not). But order can easily be changed later on if desired.
- The new table has links to stringr's pkgdown reference when built by pkgdown.
- Add missing `str_extract_all()` base R equivalent
- Fix typo in fn name.
- Use comment-style in-body YAML chunk opts
- Cosmetic changes
.Rbuildignore Show resolved Hide resolved
"str_wrap(x)", "strwrap(x)"
)

# since downlit seems to omit tables, we manually link to pkgdown reference for HTML output
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is the problem — I think the problem is that library(stringr) isn't shown in the output, so downlit can't see it. If you put library(stringr) in a separate chunk that's shown in the output, downlit should just work. This should substantially simplify the dependencies.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not familiar with how downlit works internally (it seems quite magic!). But the table cell content is still not autolinked even when there's a visible library(stringr) in the output:

Screenshot 2022-12-06 at 22-44-49 From base R

I've tested a few combinations, none of them convinced downlit to autolink table cell contents. On the other hand, all code in text paragraphs is already properly linked without a visible library(stringr) chunk, so I think the issue lies elsewhere...

Copy link
Member

Choose a reason for hiding this comment

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

Ok, in any case, I'd rather fix this upstream, rather than manually adding the links here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would be great if this could be fixed in downlit. r-lib/downlit#67 is about a different issue though, right?

I've only added the links in the stringr column since I think they're more important when consulting this table. But I wouldn't mind if both columns were autolinked. 😄

Do you want me to remove the custom downlit-code?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, please file a downlit issue, and remove the custom downlit code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the code. And filed r-lib/downlit#165.

and explicitly arrange table by stringr fn name
(This further reveals the mess in the base R API.)
@hadley hadley merged commit e4601f7 into tidyverse:main Dec 7, 2022
@hadley
Copy link
Member

hadley commented Dec 7, 2022

Thanks!

@salim-b salim-b deleted the tweak-from-base-vignette branch December 7, 2022 23:22
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