search

KentonWhite / ProjectTemplate

A template utility for R projects that provides a skeletal project.
http://projecttemplate.net
GNU General Public License v3.0
622 stars 159 forks source link

Refactor migrate.project() and fix version tests #162

Closed connectedblue closed 7 years ago

connectedblue commented 7 years ago

This PR is the first step of merging of #160.

This PR proposes a refactor of the migrate.project() logic to allow for more flexible handling in future migrations. There is no logic change in this PR, however the function now does the following:

This structure allows for config specific handling (as will be done in #160 ) and also other types of check can be performed such as checking for certain file extensions in the data directory if there has been a change to the way the reader handles the data import, for example.

This PR also contains a minor change to the test-version tests which incorrectly created a test_project in the current working directory rather than in a tempfile like the other tests. The tests themselves were not altered.

connectedblue commented 7 years ago

@KentonWhite, now that I've slept on it, I'm not happy how I've done this.

It's very brittle to emit a warning at the end, and also very confusing for the user. Also, future migration tests might make this more confusing

The only reason i did this was because existing tests broke without the warning in this function.

What I'd rather do an explicit message in the right place and update the existing tests.

What do you think?

KentonWhite commented 7 years ago

@connectedblue What tests broke without the warning function? It might be a faulty test (that happens).

connectedblue commented 7 years ago

@KentonWhite , these are the tests that fail if there is no warning emitted:

1. Failure: Version 0.5 (@test-migration.R#21) 
2. Failure: Version 0.5-2 (@test-migration.R#21)

I haven't investigated yet why they are. I'll have a look tonight to see if I can figure it out.

KentonWhite commented 7 years ago

@connectedblue I think what may be happening is that the inst/defaults/config/default.dcf file has had cache_loaded_data: TRUE added to it. This file is for Version 0.5 and is what the migration tests are checking against. If you remove the cache_loaded_data: TRUE I suspect your tests will pass.

I think the directory for this file should be renamed as it is confusing!

connectedblue commented 7 years ago

@KentonWhite I'm not sure that's the cause - on this new branch migrateproject there is no cached_loaded_data field in that file (nor in the inst/full/defaults/config directory). I created this branch directly off master and then just checked out the couple of extra files from #160 to make this PR.

Having investigated some more, this is the offending line:

expect_warning(suppressMessages(migrate.project()), "missing the following entries")

So this is kind of what I expect - the test has failed because no warning is omitted.

Let me make a couple of changes to the messages and I'll update this test to test for a message rather than a warning.

I'm making a bit of a meal of this - it's actually a lot simpler than I'm making out ....

connectedblue commented 7 years ago

Phew ... that was more tricky than it should have been. Thank goodness for the regression tests.

@KentonWhite this PR is ready for review now. The refactor of migrate.project() is complete and extra tests have been added to cater for the new user messaging which replace the old warnings.

connectedblue commented 7 years ago

Sorry @KentonWhite, I got into a pickle with git to try and align with the latest master branch which I didn't need to do, so I rolled back those changes.

Anyway, this PR is ready for review now ...

KentonWhite commented 7 years ago

@connectedblue Traveling this week. Will probably need to postpone the review until I'm back in the office.

connectedblue commented 7 years ago

Thanks @KentonWhite, enjoy your travels.

wlandau commented 7 years ago

Have you heard about the DNS hack? I'm not sure, but it may be related...

On Fri, Oct 21, 2016 at 16:32 connectedblue notifications@github.com wrote:

OK good - not sure what happened there.

Anyway, this PR is ready for review now ...

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/johnmyleswhite/ProjectTemplate/pull/162#issuecomment-255458046, or mute the thread https://github.com/notifications/unsubscribe-auth/ABgfPAPhdOVA7Znd81xRESanzDpyP7V7ks5q2SFxgaJpZM4KaX_1 .

KentonWhite commented 7 years ago

@connectedblue Changes look good.