Refactor migrate.project() and fix version tests

[connectedblue]

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:

• checks for warnings when .load.config is called
• exit if no warnings
• tell the user there are new defaults added to their existing project config
• replay the warnings (to preserve existing behaviour)
• save the new project config

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]

@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]

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

[connectedblue]

@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]

@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]

@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]

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]

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]

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

[connectedblue]

Thanks @KentonWhite, enjoy your travels.

[wlandau]

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
#162 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABgfPAPhdOVA7Znd81xRESanzDpyP7V7ks5q2SFxgaJpZM4KaX_1
.

[KentonWhite]

@connectedblue Changes look good.