Skip to content

Wrap lines longer than option "width". #390

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

Closed
wants to merge 1 commit into from

Conversation

krivit
Copy link

@krivit krivit commented Apr 24, 2018

This is WIP. References #247. Opening the pull request per #247 (comment).

@lorenzwalthert
Copy link
Collaborator

I thought a bit about this and I think in principle your approach seems to be a reasonable solution. However, I think in order for the PR to be merged, there are quite a few things I would like to change. Do you want to do that yourself or is it too tedious for you and you are happy if I do it? I am open to both. If you don't want to do it yourself, I'd ask you to close this PR and create a new PR to another (yet to be created) branch. Here's a sketch of I think needs to be done:

  • rebasing on master and resolve merge conflicts.
  • apply styler to the PR.
  • replace base R calls to apply() and friends with purrr equivalents.
  • replace the repeat() and while() statements with recursions.
  • documenting with roxygen.
  • adding unit tests and thinking of exceptions.
  • some minor code refactoring (i.e. don't define functions within functions, function re-naming, exposing of the width argument).

@krivit
Copy link
Author

krivit commented Aug 2, 2018

Thanks for looking into this. If you're willing to do the rest, I'm happy to hand it over.

I don't quite follow what you want me to do, though. Do you want me to create a new branch at krivit/styler, make no changes to it at all, and then initiate a PR from that?

@lorenzwalthert
Copy link
Collaborator

Ok, fine. No, I just meant that you create a PR to r-lib/styler#line-with, which I will merge immediately. Then, I will continue to develop from there before I merge the changes into master. If I merged this PR, it would take effect to master immediately, which I prefre not to happen.

@lorenzwalthert
Copy link
Collaborator

I just did the work for you @krivit. Welcomed to follow-up on the development and test. Thanks for the initial work.

@krivit
Copy link
Author

krivit commented Aug 4, 2018

@lorenzwalthert , I see. 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
Development

Successfully merging this pull request may close these issues.

2 participants