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

Set width argument of wrapString() call to 75 for usage section #719

Merged
merged 3 commits into from Jul 2, 2018

Conversation

@JoshOBrien
Copy link
Contributor

@JoshOBrien JoshOBrien commented Feb 28, 2018

At present, it's set to 80. Given that a further 5 space indent is added to the Usage section in the process of compiling rendered help files from Rd files, this means that the section can have width up to 85. This causes unattractive/distracting wrap around of some lines in textual help output (i.e. that got by doing help(., help_type="text")) in devices whose default viewing window is 80 characters wide.

Not sure that my proposed solution is optimal, but it involves few enough lines of code that it should be easy to review.

@hadley
Copy link
Member

@hadley hadley commented Jun 28, 2018

I like the approach.

Can you please figure out why travis is failing?

@JoshOBrien
Copy link
Contributor Author

@JoshOBrien JoshOBrien commented Jun 29, 2018

@hadley I took a look and figured out the issue. Travis failed because some of the testthat tests failed, specifically those in which roclet_process got called on a roclet without a usage section. When that happens, topic_add_usage(rd, block) gets called with rd=NULL, which ends up dispatching to wrap.string.NULL(). In my earlier pull request, I had neglected to add a width= argument to `wrap.string.NULL, so when passed a width argument, it failed with a complaint about an unused argument.

The solution is to amend my earlier pull request with one additional edit, changing the definition of wrap.string.NULL from:

wrap_string.NULL <- function(x) return(x)

to something like

wrap_string.NULL <- function(x, width = NULL) return(x)

Since I don't know the best Git way to add that patch to my earlier pull request, I'll let you deal with it by accepting the earlier pull request and then adding a fix like the one above with a second commit by hand, OK?

@hadley
Copy link
Member

@hadley hadley commented Jun 29, 2018

If you want to try making the change, you can just commit to the branch you made the changes from.

Otherwise, can you please enable edit access to your PR for me? See https://help.github.com/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork/#enabling-repository-maintainer-permissions-on-existing-pull-requests for details.

@JoshOBrien
Copy link
Contributor Author

@JoshOBrien JoshOBrien commented Jun 29, 2018

@hadley Hi -- gave it a shot and it looks from my end like my second commit got to you and passed all checks. Please let me know if I'm seeing that wrong or you need something else from me.

Thanks!

@hadley
Copy link
Member

@hadley hadley commented Jun 30, 2018

FWIW, it's a bit cleaner to provide width as an argument to the generic, rather than .... But I'll make that change locally.

To finish the PR off, can you please add a bullet to NEWS? It should briefly describe the change and end with (@yourname, #issuenumber).

JoshOBrien added 2 commits Jun 30, 2018
At present, it's set to 80. Given that a further 5 space indent is
added to the Usage section in the process of compiling rendered help
files from Rd files, this means that the section can have width up to
85. This causes unattractive/distracting wrap around of some lines in
textual help output (i.e. that got by doing `help(.,
help_type="text")`) in devices whose default viewing window is 80
characters wide.

Not sure that my proposed solution is optimal, but it involves few
enough lines of code that it should be easy to review.
@JoshOBrien
Copy link
Contributor Author

@JoshOBrien JoshOBrien commented Jun 30, 2018

@hadley Done. Thanks.

@hadley hadley merged commit 5f2322c into r-lib:master Jul 2, 2018
3 checks passed
@hadley
Copy link
Member

@hadley hadley commented Jul 2, 2018

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants