Added project.directory parameter to create.project.R

[3styleJam]

Types of change

• Bugfix (non-breaking change which fixes an issue)
• [x] New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Other (documentation etc)

Pull request checklist

• [x] Add functionality
• [x] Add tests
• [ ] Update documentation in man
• [ ] Update website documentation

---

Proposed changes

• Adding project.directory parameter to create.project(), as discussed in issue #321. I am relatively new to contributing to package repositories, so please do double check the changes and provide feedback.

[KentonWhite]

This is breaking quite a bit of the functions when no directory is given. Can you please run that tests and see where it is breaking? Looking at the logs it is breaking in these places:

• load.project
• clear.cache
• cache
• reload.project
• test.project
• stub.tests
• project.config

[3styleJam]

This is breaking quite a bit of the functions when no directory is given. Can you please run that tests and see where it is breaking? Looking at the logs it is breaking in these places:

• load.project
• clear.cache
• cache
• reload.project
• test.project
• stub.tests
• project.config

I tried test_local(path = "./tests/testthat") and this returned many failures with the same error. Figured out that one was a typo (wrote "project_directory" instead of "project.directory"), still some other errors I need to look at.

[KentonWhite]

Thanks. Let me know when the tests are passing!

[3styleJam]

Thanks. Let me know when the tests are passing!

Many of the tests create a temporary file with the name test_project before calling:

suppressMessages(create.project(test_project))

Which runs an error due to the addition of project.directory = getwd() within create.project(). I have replaced every instance of the above with:

suppressMessages(create.project(basename(test_project), project.directory = dirname(test_project)))

where applicable. There are now no test fails, however the build still fails due to warnings that I am not sure how to fix. Some look to be deprecated functions within testthat, others show e.g. path[1]="test_project55e32a50963d": No such file or directory. I'll have another think, but feel free to have a look in the meantime and let me know if you spot the issue. Thanks

[3styleJam]

I've made some more progress and now all that's left is 6 warnings. They are all within "test-migration.R" and they all happen at a call:

expect_warning(suppressMessages(load.project()), ...)

With the dots being some regular expression:

1. "migrate.project" - There doesn't seem to be any warning message produced by any function in the package that contains "migrate.project", so I am not sure why this test exists at test-migration.R:16:13
2. "missing the following entries: tables_type" and "contains the following unused entries: data_tables" - At test-migration.R:261:5 and test-migration.R:262:5 . I am not sure why this is triggering a warning.

[KentonWhite]

Great progress! I'll wait for the last few errors in migrate.project to be fixed and then prep from CRAN. Thanks for your work.

[KentonWhite]

@3styleJam I'm wanting to push a new version to CRAN soon. Should I wait for the PR to be finished or push without it?

[3styleJam]

@3styleJam I'm wanting to push a new version to CRAN soon. Should I wait for the PR to be finished or push without it?

@KentonWhite Sorry for the delay. I'm still figuring out the fix for the remaining warnings and I've also been busy elsewhere, so you can go ahead and push the new version the CRAN without this PR.

[KentonWhite]

@3styleJam if you are working on this I can wait. Like to have new features at once rather than 2 releases close to each other.

[3styleJam]

@3styleJam if you are working on this I can wait. Like to have new features at once rather than 2 releases close to each other.

I understand. I am struggling to debug this section of test-migration.R :

lapply(
    c("0.5", "0.5-2"),
    function(version) {
        test_that(paste("Version", version), {
            projdir <- tempfile("PT-test-")
            dir.create(projdir)

            file.copy(dir(file.path("migration", version), full.names = T), projdir, recursive = TRUE)
            oldwd <- setwd(projdir)
            on.exit(setwd(oldwd), add = TRUE)

            expect_warning(suppressMessages(load.project()), "migrate.project")
            on.exit(.unload.project(), add = TRUE)

            expect_message(migrate.project(), "file was missing entries")
            expect_no_warning(suppressMessages(load.project()))
            expect_defaults(get.project()$config)
        })
    }
)

More specifically at expect_warning(suppressMessages(load.project()), "migrate.project"), which flags several warnings at the config <- .load.config(override.config) call. Have there been any changes to the configuration files within the migration folder? The warnings that are showing up for me are "Your configuration contains the following unused entries..." and "Your configuration file is missing the following entries...". I am also struggling to see how these warnings have been caused by the addition of the project.directory argument. Have there been issues passing these specific tests in the past?

[3styleJam]

@KentonWhite I think I have managed to clear the remaining warnings, however I did this by modifying a couple of tests which you may want to double check (see commit c32160e) . I have not updated man (All the .Rd's say not to manually edit them) nor the website documentation. Please let me know if you'd like me to do anything further, thanks.

P.S. @gisler may be interested in reviewing the modifications to the tests within test-cache.R.

[KentonWhite]

Thanks @3styleJam I'm travelling this week will look into merging next week. The build is failing due to warnings. I'll see if I can fix up those warnings (not sure yet what is causing the warnings)

[gisler]

@3styleJam Sure, I'll take a look at it this weekend.

[gisler]

Hi @3styleJam!

I reviewed your changes to "test-cache.R" and they look good to me. :)

Just one remark concerning the new project.directory parameter: You added it right after the project.name parameter at the second position. This might break existing code in case a user relies on argument positions. That's why I usually add new parameters to the end of the existing parameters list of a function, but before a possible .... However, one could also argue that it is bad practice to rely on argument positions. @KentonWhite You have to decide on this. ;)

Besides, @KentonWhite, since there is a mix of different coding styles in the package by now, which kinda forces contributers to change more than necessary ... What do you think of applying the new formatter Air to the existing code base?

[3styleJam]

Hi @3styleJam!

I reviewed your changes to "test-cache.R" and they look good to me. :)

Just one remark concerning the new project.directory parameter: You added it right after the project.name parameter at the second position. This might break existing code in case a user relies on argument positions. That's why I usually add new parameters to the end of the existing parameters list of a function, but before a possible .... However, one could also argue that it is bad practice to rely on argument positions. @KentonWhite You have to decide on this. ;)

Besides, @KentonWhite, since there is a mix of different coding styles in the package by now, which kinda forces contributers to change more than necessary ... What do you think of applying the new formatter Air to the existing code base?

Thanks @gisler . Good point concerning the parameter position. I placed it second as it seemed appropriate to put next to project.name, but since it is not a required argument for the function to execute then I am equally happy it being placed at the last position too (I rely on argument positions when writing code too). Nice to suggest Air to format the code too, if not that then styler and lintr.

[gisler]

Ah, styler. Thanks for reminding me about this package.