Rename `root` parameter to `path`. #758

bdice · 2022-05-10T14:45:25Z

Description

See #757.

Motivation and Context

Resolves #757.

Checklist:

I am familiar with the Contributing Guidelines.
I agree with the terms of the Contributor Agreement.
My name is on the list of contributors.
The changes introduced by this pull request are covered by existing or newly introduced tests.
The package documentation and framework documentation in signac-docs are up to date with these changes.
I have updated the changelog and added any related issue and pull request numbers for future reference.

signac/common/config.py

cbkerr

There's still a bit of old terminology (like "job workspace") that needs to be fixed.

changelog.txt

cbkerr · 2022-05-12T01:38:34Z

signac/contrib/project.py

@@ -78,13 +78,13 @@ class Project:
    A :class:`Project` may only be constructed in a directory that is already a
    signac project, i.e. a directory in which :func:`~signac.init_project` has
    already been run. If project discovery (searching upwards in the folder
-    hierarchy until a project root is discovered) is desired, users should
+    hierarchy until a project is discovered) is desired, users should


Can we say how a project is discovered? Like the presence of a signac.rc file?

I would need to consult the implementation to refresh myself but I think it's defined by the existence of a .signac/config file (?). Either way, it may be better to write this definition in one place (not in every method/function docstring), and it's outside the scope of this PR which is focused on an API change.

Agreed https://github.com/orgs/glotzerlab/projects/5#card-81869156

signac/contrib/project.py

cbkerr · 2022-05-12T01:42:11Z

signac/contrib/project.py

@@ -1370,7 +1370,7 @@ def _read_cache(self):
    def temporary_project(self, dir=None):
        """Context manager for the initialization of a temporary project.

-        The temporary project is by default created within the root project's
+        The temporary project is by default created within the parent project's


"Parent project" doesn't sound the same as "root" in this context. To me, "parent" sounds like a higher level then the project. This could be confusing once we have nested projects

Well... it is sort of a parent. The temporary project is created in a subdirectory of the project instance calling this. This is awkward almost any way I can think of writing it, since it's hard to explain the benefit of this implementation detail without describing specific filesystem hierarchies.

Proposal to also rewrite the beginning of the docstring to be:

def temporary_project(self, dir=None): ""Context manager that initializes a temporary project within the calling Project. The temporary project path defaults to that of the calling project workspace. ... """

I think this helps convey the relation better and uses more consistent terms.

btw - we should rename the argument from dir to path.

This PR is focused on changing root to path, primarily in the get_project and init_project functions. A follow-up PR to address this would be appropriate.

I agree with your suggestions and I agree that the temporary project functionality and docs need a careful look before releasing 2.0. I just don't think this PR is the right place to iterate on that.

cbkerr · 2022-05-12T01:44:39Z

signac/contrib/project.py

+            the specified directory, otherwise only return projects with
+            a directory whose path is identical to the specified path
+            argument (Default value = True).


I would like to see the updated signac-docs page on what a project is now. See my other comment about whether a project "has" a path.
(not blocking, but for curiosity and to start explaining to users)

signac/contrib/project.py

tests/test_project.py

Co-authored-by: Corwin Kerr <[email protected]>

cbkerr

Check 2 rewordings in this commit: 74d7f78
plus this comment: #758 (comment)

signac/contrib/job.py

bdice · 2022-05-13T04:20:29Z

I'm not sure if 74d7f78 is quite accurate. If signac.get_job() is called from a subdirectory of a job directory, e.g. my_project/workspace/4abbdfd8bbd68bb88e726bb8978ffa14/asdf/, signac.get_job() will return the job from 4abbdf....

Perhaps it should say something like:
The path from which to search. The first job found in or above this path is returned.

cbkerr

Thanks for bearing with me!
Notes for future PRs:

Functionality and description of Project.temporary_project
Review docstrings for consistent terminology.

syjlee

Looks good to me. It reminds me of where's waldo

* Change root to path. * Update tests to use path instead of root. * Update benchmarks to use path instead of root. * Update changelog. * Update changelog.txt Co-authored-by: Corwin Kerr <[email protected]> * Update signac/contrib/project.py Co-authored-by: Corwin Kerr <[email protected]> * Update tests/test_project.py Co-authored-by: Corwin Kerr <[email protected]> * Remove use of root. * Refactor docs for fn/isfile methods. * Bradley check these rewordings * Replace outdated job workspace directory * One more outdated job workspace directory * Apply minor changes * Describe path argument searching Co-authored-by: Corwin Kerr <[email protected]>

bdice added 3 commits May 10, 2022 09:42

Change root to path.

9fd3715

Update tests to use path instead of root.

4433d73

Update benchmarks to use path instead of root.

733b484

bdice requested review from a team as code owners May 10, 2022 14:45

bdice requested review from vyasr and syjlee and removed request for a team May 10, 2022 14:45

Update changelog.

53694b0

bdice changed the base branch from master to next May 10, 2022 14:48

bdice added this to the v2.0.0 milestone May 10, 2022

bdice self-assigned this May 10, 2022

bdice mentioned this pull request May 10, 2022

Rename root parameter for all Project methods to path #757

Closed

syjlee reviewed May 10, 2022

View reviewed changes

signac/common/config.py Outdated Show resolved Hide resolved

cbkerr requested changes May 12, 2022

View reviewed changes

bdice and others added 5 commits May 12, 2022 15:18

Update changelog.txt

7808bed

Co-authored-by: Corwin Kerr <[email protected]>

Update signac/contrib/project.py

09015db

Co-authored-by: Corwin Kerr <[email protected]>

Update tests/test_project.py

a88e782

Co-authored-by: Corwin Kerr <[email protected]>

Remove use of root.

ba109a2

Refactor docs for fn/isfile methods.

de6d897

bdice requested a review from cbkerr May 12, 2022 22:49

cbkerr added 3 commits May 12, 2022 19:56

Bradley check these rewordings

74d7f78

Replace outdated job workspace directory

14e96b6

One more outdated job workspace directory

6e03f71

cbkerr requested changes May 13, 2022

View reviewed changes

signac/contrib/job.py Outdated Show resolved Hide resolved

signac/contrib/job.py Outdated Show resolved Hide resolved

signac/contrib/job.py Outdated Show resolved Hide resolved

cbkerr added 2 commits May 13, 2022 16:04

Apply minor changes

2ebbad2

Describe path argument searching

52fda9b

cbkerr approved these changes May 13, 2022

View reviewed changes

syjlee approved these changes May 13, 2022

View reviewed changes

bdice merged commit ea3b4df into next May 14, 2022

bdice deleted the root-to-path branch May 14, 2022 01:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename `root` parameter to `path`. #758

Rename `root` parameter to `path`. #758

bdice commented May 10, 2022

cbkerr left a comment

cbkerr May 12, 2022

bdice May 12, 2022

cbkerr May 12, 2022

cbkerr May 12, 2022 •

edited

Loading

bdice May 12, 2022 •

edited

Loading

cbkerr May 12, 2022 •

edited

Loading

bdice May 13, 2022 •

edited

Loading

cbkerr May 12, 2022 •

edited

Loading

cbkerr left a comment •

edited

Loading

bdice commented May 13, 2022

cbkerr left a comment •

edited

Loading

syjlee left a comment

Rename root parameter to path. #758

Rename root parameter to path. #758

Conversation

bdice commented May 10, 2022

Description

Motivation and Context

Checklist:

cbkerr left a comment

Choose a reason for hiding this comment

cbkerr May 12, 2022

Choose a reason for hiding this comment

bdice May 12, 2022

Choose a reason for hiding this comment

cbkerr May 12, 2022

Choose a reason for hiding this comment

cbkerr May 12, 2022 • edited Loading

Choose a reason for hiding this comment

bdice May 12, 2022 • edited Loading

Choose a reason for hiding this comment

cbkerr May 12, 2022 • edited Loading

Choose a reason for hiding this comment

bdice May 13, 2022 • edited Loading

Choose a reason for hiding this comment

cbkerr May 12, 2022 • edited Loading

Choose a reason for hiding this comment

cbkerr left a comment • edited Loading

Choose a reason for hiding this comment

bdice commented May 13, 2022

cbkerr left a comment • edited Loading

Choose a reason for hiding this comment

syjlee left a comment

Choose a reason for hiding this comment

Rename `root` parameter to `path`. #758

Rename `root` parameter to `path`. #758

cbkerr May 12, 2022 •

edited

Loading

bdice May 12, 2022 •

edited

Loading

cbkerr May 12, 2022 •

edited

Loading

bdice May 13, 2022 •

edited

Loading

cbkerr May 12, 2022 •

edited

Loading

cbkerr left a comment •

edited

Loading

cbkerr left a comment •

edited

Loading