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

new use_tidy_style() #197

Merged
merged 22 commits into from Jan 17, 2018
Merged

new use_tidy_style() #197

merged 22 commits into from Jan 17, 2018

Conversation

@lorenzwalthert
Copy link
Contributor

@lorenzwalthert lorenzwalthert commented Jan 8, 2018

Closes #72. I am not very familiar with the usethis source code, so let me know if there were some conventions I did not follow. Also, I am not sure if the warning (which I just copied from styler) in the documentation of the function is needed.
cc: @krlmlr.

@jennybc
Copy link
Member

@jennybc jennybc commented Jan 8, 2018

Just took a quick look. One thing I already know it needs is a check re: clean git status. See the source for use_dev_version() for some inspiration.

@lorenzwalthert
Copy link
Contributor Author

@lorenzwalthert lorenzwalthert commented Jan 8, 2018

thanks, I will do so.

@jennybc
Copy link
Member

@jennybc jennybc commented Jan 8, 2018

Can you also take a look at the AppVeyor failure?

@lorenzwalthert
Copy link
Contributor Author

@lorenzwalthert lorenzwalthert commented Jan 8, 2018

Yes, I try to understand what's going on because locally, with devtools::test() I get no errors. Some path issues I suppose. The problem is that I also don't fully understand scoped_temporary_package().
As far as the styling goes: In #72, it was suggested to basically alias styler::style_pkg(). However, usethis can also be used for non-package projects, right? So we should probably dispatch internally between style_pkg() and style_dir().

@jennybc
Copy link
Member

@jennybc jennybc commented Jan 8, 2018

However, usethis can also be used for non-package projects, right? So we should probably dispatch internally between style_pkg() and style_dir().

Yep, agree.

Yes, I try to understand what's going on because locally, with devtools::test() I get no errors. Some path issues I suppose.

It doesn't smell like a path problem FWIW. Also we're getting same failure on Travis and AppVeyor now. I think it's legit.

@lorenzwalthert
Copy link
Contributor Author

@lorenzwalthert lorenzwalthert commented Jan 8, 2018

For the uncommitted changes- test: I wondered whether I should create a function check_uncommitted_changes() instead of just c/p the code. Should I define it under R/helpers.R?

@jennybc
Copy link
Member

@jennybc jennybc commented Jan 8, 2018

For the uncommitted changes- test: I wondered whether I should create a function check_uncommitted_changes() instead of just c/p the code. Should I define it under R/helpers.R?

That's a good idea. I suggest you define it in R/git-utils.R.

@lorenzwalthert
Copy link
Contributor Author

@lorenzwalthert lorenzwalthert commented Jan 8, 2018

Ok, I took a look at use_dev_version() and tried to get an idea on how things should look. However, I still don't understand why the CI builds fail. I even get perfect R CMD CHECK and perfect rcmdcheck::rcmdcheck() locally and changed a few things but it still fails on travis and Appveyor.

@lorenzwalthert
Copy link
Contributor Author

@lorenzwalthert lorenzwalthert commented Jan 8, 2018

Also re-installed relevant dependencies from CRAN to make sure they match the CI configuration.

@lorenzwalthert
Copy link
Contributor Author

@lorenzwalthert lorenzwalthert commented Jan 8, 2018

Once the tests work I am happy to test case for a project that is not a package.

@jennybc
Copy link
Member

@jennybc jennybc commented Jan 8, 2018

It's not clear to me what the problem is. Suggest you remove the capture_output()s in the test to see if we learn anything useful from seeing a bit more output. You might also use simpler if()...else() logic instead of the do.call(). Will comment on the code itself ...

NAMESPACE Outdated
export(use_tidy_support)
export(use_tidy_versions)
export(use_travis)
export(use_usethis)
export(use_vignette)
importFrom(styler,style_pkg)
importFrom(styler,tidyverse_style)
Copy link
Member

@jennybc jennybc Jan 8, 2018

We generally don't do this.

Copy link
Contributor Author

@lorenzwalthert lorenzwalthert Jan 8, 2018

So should I just use :: whenever I need an external function?

Copy link
Member

@jennybc jennybc Jan 8, 2018

Yes.

R/styler.R Outdated
check_is_package("use_tidy_style()")
check_uncommitted_changes()
styled <- do.call(
ifelse(is_package(), "style_pkg", "style_dir"),
Copy link
Member

@jennybc jennybc Jan 8, 2018

I wouldn't use ifelse() this way.

In fact, I'd unwrap this into a more conventional if() ... else() construct.

Copy link
Contributor Author

@lorenzwalthert lorenzwalthert Jan 8, 2018

Ok.

@lorenzwalthert
Copy link
Contributor Author

@lorenzwalthert lorenzwalthert commented Jan 8, 2018

Sorry I did not realize earlier but maybe we should move use_tidy_style() to the other tidy_* functions in R/tidyverse.R and do the same with tests.

@lorenzwalthert
Copy link
Contributor Author

@lorenzwalthert lorenzwalthert commented Jan 12, 2018

For some reason the styling throws an error. Unfortunately, the transformation of the code within style_pkg() is done with enc::transform_lines_enc() so we can't easily print the source of the problem by changing the source code of usethis. Any change anyone could try to run the tests locally and see if they get the same problem too?

package = capture_output(create_package(dir, rstudio = rstudio, open = FALSE)),
project = capture_output(create_project(dir, rstudio = rstudio, open = FALSE))
package = testthat::capture_output(create_package(dir, rstudio = rstudio, open = FALSE)),
project = testthat::capture_output(create_project(dir, rstudio = rstudio, open = FALSE))
Copy link
Member

@jennybc jennybc Jan 15, 2018

These aren't necessary and aren't used elsewhere in the tests. Can you remove?

In terms of test development, I load testthat in all interactive sessions in .Rprofile FWIW.

Copy link
Contributor Author

@lorenzwalthert lorenzwalthert Jan 15, 2018

Yes they are not necessary if testthat is loaded. I thought it might be good practice to not assume this is the case so I added them. But I will remove them in that case. As it stands if now, the PR is anyways not intended to be merged, the primary goal was to make the unit tests passing, which was not possible.

Copy link
Contributor Author

@lorenzwalthert lorenzwalthert Jan 17, 2018

ok, removed.

@jennybc
Copy link
Member

@jennybc jennybc commented Jan 15, 2018

I'm about to run the tests locally on Windows and see if I learn anything.

@lorenzwalthert
Copy link
Contributor Author

@lorenzwalthert lorenzwalthert commented Jan 15, 2018

Thanks. Also, once we get them running, we can squash all the commits we needed to get there to keep a clean history I suggest.

@jennybc
Copy link
Member

@jennybc jennybc commented Jan 15, 2018

I can replicate but not explain the failure on Windows. But only via devtools::check(). Tests pass with devtools::test() and via R CMD build + R CMD check. It's a peculiar combination of results.

If I replace use_tidy_style() in the test with write_utf8(path_to_bad_style, "a <- 2\n"), tests pass. So this is a true result, in some sense. I think the file really is not getting restyled but I don't know why.

@jennybc
Copy link
Member

@jennybc jennybc commented Jan 15, 2018

You might need to make a debugging branch of styler that emits more info from style_pkg() and maybe prettify_pkg() and use the Remotes field in DESCRIPTION to install styler from that branch.

Alternatively, you could try to avoid the use of scoped_temporary_package() in your test, i.e. create the bare minimum file structure to convince these functions to work. Like, just fake making a package. Part of me wonders if this has something to do with withr, which is used in scoped_temporary_package() and in style_pkg().

Update: re avoiding scoped_temporary_package(). You could probably just inline a lot of that logic into your test, in this experiment. Let's try to eliminate withr as our problem.

@lorenzwalthert
Copy link
Contributor Author

@lorenzwalthert lorenzwalthert commented Jan 15, 2018

Ok. Thanks for helping me out with that. As you said, it seems really strange. I hope I can find time soon to look at it more in depth and follow some of the suggestions you outlined above to find the root of the problem. It would be nice if we could continue to use some of the existing infrastructure eventually though. I did also not think of withr as a potential cause, thanks for that hint.

@jennybc
Copy link
Member

@jennybc jennybc commented Jan 15, 2018

If it is a bad interaction between these two, nested uses of withr, that would be interesting to know. At this point, I suspect use_tidy_style() works just fine and we're now learning something about our tooling for package development and testing. But we can't merge this until we can test it. By the way, this should test the "non-package project" branch too, before we're done.

@jimhester
Copy link
Member

@jimhester jimhester commented Jan 17, 2018

The failures are because you have an implicit dependency on dplyr since you are using map_dfr()in styler

And purrr has a dependency on dplyr for map_dfr().

IMHO you need to remove the map_dfr() calls in styler (and remove dplyr from Suggests: in styler, so that these errors surface sooner)

@lorenzwalthert
Copy link
Contributor Author

@lorenzwalthert lorenzwalthert commented Jan 17, 2018

Thanks so much @jimhester, I would never have considered that. I assumed we have removed the dplyr dependency completely with r-lib/styler#182 but it was apparently only after that (as you linked above) where I introduced it again without taking notice. I'll try to fix it.

@lorenzwalthert
Copy link
Contributor Author

@lorenzwalthert lorenzwalthert commented Jan 17, 2018

@jennybc might that also affect the upcoming reprex release with styler in Suggests: because the dplyr dependency is missing? Also, it seems quite a coincidence that we found out about this problem because only if the expression to style contains an EQ_ASSIGN token (like in a = 2 that I implemented in a test), purrr::map_dfr() is used.

@lorenzwalthert
Copy link
Contributor Author

@lorenzwalthert lorenzwalthert commented Jan 17, 2018

I opened r-lib/styler#323 to discuss the implicit dplyr dependency and implications in general.

@jennybc
Copy link
Member

@jennybc jennybc commented Jan 17, 2018

might that also affect the upcoming reprex release with styler in Suggests

I now regard this as completely a styler problem 😁

Realistically, the vast vast majority of usethis and reprex users will have dplyr. So I won't let this block a CRAN update to usethis or reprex. Nothing has gotten worse, now we just know about a pre-existing problem.

But it would be good to straighten things out over in styler! And get rid of the dplyr dependency.

I propose that you change the test here to something that doesn't exercise the dplyr-calling code in styler, so we can move forward. If you're really fast, it can happen before we submit usethis -- TODAY.

@jennybc
Copy link
Member

@jennybc jennybc commented Jan 17, 2018

Any test that will make git commits needs to have this in it:

# git2r::git2r::discover_repository() not working on R 3.1 (Travis)
skip_if(getRversion() < 3.2)
skip_if_no_git_config()

@lorenzwalthert
Copy link
Contributor Author

@lorenzwalthert lorenzwalthert commented Jan 17, 2018

Ok, I think that's it. In particular, I reverted the testthat::capture_output() mask I introduced and added a test for a project.
Let me know what you think. I know there are many commits now so can we just squash and merge at the end? Otherwise I can do interactive rebase and try to squash individual commits together but if that's not required from your side will spare this effort 😊.

@jennybc
Copy link
Member

@jennybc jennybc commented Jan 17, 2018

This will be a squash and merge regardless. Let me just look it all over again.

@lorenzwalthert
Copy link
Contributor Author

@lorenzwalthert lorenzwalthert commented Jan 17, 2018

Ok, sure.

R/dev-version.R Outdated
call. = FALSE
)
}
check_uncommitted_changes(proj_get())
Copy link
Member

@jennybc jennybc Jan 17, 2018

Pretty sure this can just be check_uncommitted_changes(), right?

Copy link
Contributor Author

@lorenzwalthert lorenzwalthert Jan 17, 2018

Yes, I was not sure because it appeared to me that in some places the path was set while in other places it was not, so I thought I go the safe way and add it. Will remove it then.

R/styler.R Outdated
)
}
cat_line()
done("Styled package according to the tidyverse style guide")
Copy link
Member

@jennybc jennybc Jan 17, 2018

Propose: "Project has been styled according to ..." to be less package specific.

Copy link
Contributor Author

@lorenzwalthert lorenzwalthert Jan 17, 2018

Oh yes.

@jennybc
Copy link
Member

@jennybc jennybc commented Jan 17, 2018

Green checks on travis!

Just change those two things.

I'm editing NEWS right now so will anticipate this for you.

@lorenzwalthert
Copy link
Contributor Author

@lorenzwalthert lorenzwalthert commented Jan 17, 2018

Done. On a related note, I think there are quite help files that are not up-to-date with the package source code at that stage but I have not committed anything because it's not related to this PR.

@jennybc
Copy link
Member

@jennybc jennybc commented Jan 17, 2018

I think there are quite help files that are not up-to-date with the package source code

I've been documenting a lot, so if you have concrete examples, please pass along. I'm not aware of a problem.

@jennybc
Copy link
Member

@jennybc jennybc commented Jan 17, 2018

Thanks!

@jennybc jennybc merged commit 2abb042 into r-lib:master Jan 17, 2018
4 checks passed
@lorenzwalthert
Copy link
Contributor Author

@lorenzwalthert lorenzwalthert commented Jan 17, 2018

Thanks 😊. Ok, I think the reason for that was simply different roxygen versions (I don't use the dev version) and my changes had made things worse, so I think it's all good.

@lorenzwalthert lorenzwalthert deleted the styler branch Jan 17, 2018
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

3 participants