Change Project constructor to use root directory #706
Conversation
vyasr
requested review from
kidrahahjo and
Tobias-Dwyer
and removed request for
a team
Codecov Report
@@ Coverage Diff @@
## schema2 #706 +/- ##
===========================================
+ Coverage 86.24% 86.28% +0.04%
===========================================
Files 52 52
Lines 5037 5038 +1
Branches 1099 1099
===========================================
+ Hits 4344 4347 +3
+ Misses 491 490 -1
+ Partials 202 201 -1
Continue to review full report at Codecov.
|
| @@ -1608,7 +1609,7 @@ def get_project(cls, root=None, search=True, **kwargs): | |||
| ) | |||
| ) | |||
|
|
|||
| return cls(config=config, **kwargs) | |||
| return cls(root=config["project_dir"], **kwargs) | |||
There was a problem hiding this comment.
The current approach finds and loads a project config and then uses its "project_dir" key to construct the Project. Can this be simplified somehow, so that we check for config existence and then pass the root to the constructor? We shouldn't need to load the config twice (in get_project and again in the constructor), right?
There was a problem hiding this comment.
Hmm that's a good point. This question is related to the last piece of #643, which is changing load_config to not populate the project_dir into the config but to instead change load_config to return a tuple of (root_dir, config). The reason I originally wanted to make that change is because actually modifying the contents of the config with something that isn't contained in any of the config files is an unexpected side effect that could cause confusion. Perhaps your question here is an indication that the discovery aspect should be separated into a completely separate function, and that load_config would stop discovering altogether? Alternatively, load_config could still be allowed to discover even if we added a separate discover_config function, but that seems like unnecessary redundancy.
There was a problem hiding this comment.
@bdice let me know what you think here. I can split those changes into a separate PR if that's the direction that we want to go.
There was a problem hiding this comment.
Yes, it might be a better design to separate discovery from config loading (parsing). A separate PR might be best, I agree.
* Change project constructor to accept a root directory instead of a config file. * Change Project repr. * Address easy PR comments.
* Change project constructor to accept a root directory instead of a config file. * Change Project repr. * Address easy PR comments.
* Change project constructor to accept a root directory instead of a config file. * Change Project repr. * Address easy PR comments.
* Initial migration to schema version 2 including config rename (#678) * Implement initial migration to schema version 2. * Require project-local config to be signac.rc (not .signacrc) and make searches stricter to match. * Standardize method for getting project config at a root. * Move config to .signac/config. * Fix import order. * Address PR comments. * Remove some unnecessary code. * Address final PR coments. * Use integer schema version numbers (#688) * Change schema versioning to use integer strings. * Switch from int strings to pure ints. * Update signac/contrib/migration/__init__.py Co-authored-by: Bradley Dice <[email protected]> Co-authored-by: Bradley Dice <[email protected]> * Remove project name from schema (#684) * Remove project id API. * Remove project name from config as part of migration. * Fix issues with config CLI and remove project from default cfg. * Address PR comments. * Change the str of a project to the str of its root directory. * Change Project constructor to use root directory (#706) * Change project constructor to accept a root directory instead of a config file. * Change Project repr. * Address easy PR comments. * Move internal signac files into .signac subdirectory (#708) * Move shell history. * Move sp_cache file. * Address PR comments. * Move discovery to separate functions. (#711) * Move discovery to separate functions. * Address first round of PR comments. * Address PR comments. * Apply suggestions. * Remove configurable workspace directory (#714) * Remove workspace configurability. * Implement workspace_dir migration. * Apply suggestions from code review Co-authored-by: Bradley Dice <[email protected]> * Address remaining PR comments. * Update tests/test_project.py * Remove mention of configurability from project workspace docstring * Address PR comments. Co-authored-by: Bradley Dice <[email protected]> Co-authored-by: Corwin Kerr <[email protected]> * Update description of schema migration. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci Co-authored-by: Bradley Dice <[email protected]> Co-authored-by: Corwin Kerr <[email protected]> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
* Change project constructor to accept a root directory instead of a config file. * Change Project repr. * Address easy PR comments.
* Change project constructor to accept a root directory instead of a config file. * Change Project repr. * Address easy PR comments.
* Change project constructor to accept a root directory instead of a config file. * Change Project repr. * Address easy PR comments.
* Change project constructor to accept a root directory instead of a config file. * Change Project repr. * Address easy PR comments.
* Change project constructor to accept a root directory instead of a config file. * Change Project repr. * Address easy PR comments.
* Change project constructor to accept a root directory instead of a config file. * Change Project repr. * Address easy PR comments.
* Change project constructor to accept a root directory instead of a config file. * Change Project repr. * Address easy PR comments.
Description
The Project constructor no longer requires a config object. Projects are constructed using a root directory alone.
Motivation and Context
Part of #643. Since projects no longer have ids and are entirely identified by their root directory in signac 2.0, the constructor can be simplified to reflect that.
Types of Changes
1The change breaks (or has the potential to break) existing functionality.
Checklist:
If necessary: