Repository: glotzerlab/signac
Pull request #856 (2022-11-06 20:13:38)

Update v2 migration logic.
## Description
This PR makes v1 -> v2 schema migration logic a little more robust. I fixed a bug where the `[y/N]` prompt would disappear if the user pressed backspace. I also switched to using `os.path.join` because, unlike in the cases where we use `os.sep.join` internally, we don't need to sacrifice the checks that `os.path.join` does for speed. We typically put explanatory comments anywhere we use `os.sep.join` to be explicit about the performance needs, which are not relevant for the migration script.

(This does not require a changelog entry because the corresponding migration code has not yet been released.)

## Checklist:
- [x] I am familiar with the [Contributing Guidelines](https://github.com/glotzerlab/signac/blob/master/CONTRIBUTING.md).
- [x] I agree with the terms of the [Contributor Agreement](https://github.com/glotzerlab/signac/blob/master/ContributorAgreement.md).
- [x] My name is on the [list of contributors](https://github.com/glotzerlab/signac/blob/master/contributors.yaml).
- [x] The changes introduced by this pull request are covered by existing or newly introduced tests.
- [ ] The [package documentation](https://github.com/glotzerlab/signac/tree/master/doc) and [framework documentation](https://docs.signac.io/) in [signac-docs](https://github.com/glotzerlab/signac-docs) are up to date with these changes.
- [ ] I have updated the [changelog](https://github.com/glotzerlab/signac/blob/master/changelog.txt) and added any related issue and pull request numbers for future reference.


codecov[bot]: # [Codecov](https://codecov.io/gh/glotzerlab/signac/pull/856?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glotzerlab) Report
> Merging [#856](https://codecov.io/gh/glotzerlab/signac/pull/856?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glotzerlab) (045c6d2) into [master](https://codecov.io/gh/glotzerlab/signac/commit/76bba331a22c0479a6eb5b37e571fb9666a79fe2?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glotzerlab) (76bba33) will **decrease** coverage by `8.02%`.
> The diff coverage is `83.33%`.

```diff
@@            Coverage Diff             @@
##           master     #856      +/-   ##
==========================================
- Coverage   85.02%   76.99%   -8.03%     
==========================================
  Files          50       50              
  Lines        4668     4669       +1     
  Branches      924      925       +1     
==========================================
- Hits         3969     3595     -374     
- Misses        519      898     +379     
+ Partials      180      176       -4     
```


| [Impacted Files](https://codecov.io/gh/glotzerlab/signac/pull/856?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glotzerlab) | Coverage Δ | |
|---|---|---|
| [signac/common/config.py](https://codecov.io/gh/glotzerlab/signac/pull/856/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glotzerlab#diff-c2lnbmFjL2NvbW1vbi9jb25maWcucHk=) | `89.39% <ø> (ø)` | |
| [signac/contrib/project.py](https://codecov.io/gh/glotzerlab/signac/pull/856/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glotzerlab#diff-c2lnbmFjL2NvbnRyaWIvcHJvamVjdC5weQ==) | `82.84% <ø> (-7.11%)` | :arrow_down: |
| [signac/contrib/utility.py](https://codecov.io/gh/glotzerlab/signac/pull/856/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glotzerlab#diff-c2lnbmFjL2NvbnRyaWIvdXRpbGl0eS5weQ==) | `66.66% <ø> (ø)` | |
| [signac/contrib/migration/v1\_to\_v2.py](https://codecov.io/gh/glotzerlab/signac/pull/856/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glotzerlab#diff-c2lnbmFjL2NvbnRyaWIvbWlncmF0aW9uL3YxX3RvX3YyLnB5) | `88.37% <83.33%> (-2.11%)` | :arrow_down: |
| [signac/core/h5store.py](https://codecov.io/gh/glotzerlab/signac/pull/856/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glotzerlab#diff-c2lnbmFjL2NvcmUvaDVzdG9yZS5weQ==) | `26.55% <0.00%> (-64.49%)` | :arrow_down: |
| [signac/core/dict\_manager.py](https://codecov.io/gh/glotzerlab/signac/pull/856/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glotzerlab#diff-c2lnbmFjL2NvcmUvZGljdF9tYW5hZ2VyLnB5) | `33.72% <0.00%> (-54.66%)` | :arrow_down: |
| [...\_synced\_collections/backends/collection\_mongodb.py](https://codecov.io/gh/glotzerlab/signac/pull/856/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glotzerlab#diff-c2lnbmFjL19zeW5jZWRfY29sbGVjdGlvbnMvYmFja2VuZHMvY29sbGVjdGlvbl9tb25nb2RiLnB5) | `52.27% <0.00%> (-38.64%)` | :arrow_down: |
| [signac/\_synced\_collections/numpy\_utils.py](https://codecov.io/gh/glotzerlab/signac/pull/856/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glotzerlab#diff-c2lnbmFjL19zeW5jZWRfY29sbGVjdGlvbnMvbnVtcHlfdXRpbHMucHk=) | `64.00% <0.00%> (-36.00%)` | :arrow_down: |
| [...ac/\_synced\_collections/backends/collection\_zarr.py](https://codecov.io/gh/glotzerlab/signac/pull/856/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glotzerlab#diff-c2lnbmFjL19zeW5jZWRfY29sbGVjdGlvbnMvYmFja2VuZHMvY29sbGVjdGlvbl96YXJyLnB5) | `54.00% <0.00%> (-34.00%)` | :arrow_down: |
| [signac/core/utility.py](https://codecov.io/gh/glotzerlab/signac/pull/856/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glotzerlab#diff-c2lnbmFjL2NvcmUvdXRpbGl0eS5weQ==) | `71.42% <0.00%> (-28.58%)` | :arrow_down: |
| ... and [13 more](https://codecov.io/gh/glotzerlab/signac/pull/856/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glotzerlab) | |

Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glotzerlab). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glotzerlab)


Patch:
diff --git a/changelog.txt b/changelog.txt
index 5a3e7199..3733d03d 100644
--- a/changelog.txt
+++ b/changelog.txt
@@ -60,6 +60,7 @@ Removed
  - The public API of the ``synced_collection`` subpackage (#807, #790).
  - The warnings module (#815).
  - The signac.common.validate module (#853).
+ - The ``Config`` class (#860).
  - The ``DictManager`` class (#858).
 
 Version 1
diff --git a/setup.cfg b/setup.cfg
index 136e44a8..9d7a4113 100644
--- a/setup.cfg
+++ b/setup.cfg
@@ -32,6 +32,7 @@ parallel = True
 source = signac
 omit = 
 	*/signac/common/configobj/*.py
+	*/signac/common/deprecation/*.py
 
 [tool:pytest]
 filterwarnings = 
diff --git a/signac/__main__.py b/signac/__main__.py
index 576512d7..388f6430 100644
--- a/signac/__main__.py
+++ b/signac/__main__.py
@@ -107,7 +107,7 @@ def _fmt_bytes(nbytes, suffix="B"):
         if abs(nbytes) < 1024.0:
             return f"{nbytes:3.1f} {unit}{suffix}"
         nbytes /= 1024.0
-    return "{:.1f} {}{}".format(nbytes, "Yi", suffix)
+    return f"{nbytes:.1f} Yi{suffix}"
 
 
 def _open_job_by_id(project, job_id):
@@ -133,15 +133,15 @@ def _open_job_by_id(project, job_id):
         )
 
 
-def find_with_filter_or_none(args):
+def _find_with_filter_or_none(args):
     """Return a filtered subset of jobs or None."""
     if args.job_id or args.filter:
-        return find_with_filter(args)
+        return _find_with_filter(args)
     else:
         return None
 
 
-def find_with_filter(args):
+def _find_with_filter(args):
     """Return a filtered subset of jobs."""
     if getattr(args, "job_id", None):
         if args.filter:
@@ -194,7 +194,7 @@ def main_statepoint(args):
 def main_document(args):
     """Handle document subcommand."""
     project = get_project()
-    for job_id in find_with_filter(args):
+    for job_id in _find_with_filter(args):
         job = _open_job_by_id(project, job_id)
         if args.pretty:
             pprint(job.document(), depth=args.pretty)
@@ -276,7 +276,7 @@ def main_find(args):
             return pformat(s, depth=args.pretty)
 
     try:
-        for job_id in find_with_filter(args):
+        for job_id in _find_with_filter(args):
             print(job_id)
             job = project.open_job(id=job_id)
 
@@ -304,7 +304,7 @@ def main_diff(args):
     """Handle diff subcommand."""
     project = get_project()
 
-    jobs = find_with_filter_or_none(args)
+    jobs = _find_with_filter_or_none(args)
     jobs = (
         (_open_job_by_id(project, job) for job in jobs) if jobs is not None else project
     )
@@ -322,7 +322,7 @@ def main_view(args):
     project.create_linked_view(
         prefix=args.prefix,
         path=args.path,
-        job_ids=find_with_filter(args),
+        job_ids=_find_with_filter(args),
     )
 
 
@@ -337,7 +337,7 @@ def main_schema(args):
     project = get_project()
     print(
         project.detect_schema(
-            exclude_const=args.exclude_const, subset=find_with_filter_or_none(args)
+            exclude_const=args.exclude_const, subset=_find_with_filter_or_none(args)
         ).format(
             depth=args.depth, precision=args.precision, max_num_range=args.max_num_range
         )
@@ -346,10 +346,7 @@ def main_schema(args):
 
 def main_sync(args):
     """Handle sync subcommand."""
-    # TODO: This function appears to be untested.
-    #
     # Validate provided argument combinations
-    #
     if args.archive:
         args.recursive = True
         args.links = True
@@ -388,9 +385,7 @@ def main_sync(args):
 
         filecmp._sig = _sig
 
-    #
     # Setup synchronization process
-    #
 
     source = get_project(path=args.source)
     try:
@@ -398,7 +393,7 @@ def main_sync(args):
     except LookupError:
         _print_err("WARNING: The destination does not appear to be a project path.")
         raise
-    selection = find_with_filter_or_none(args)
+    selection = _find_with_filter_or_none(args)
 
     if args.strategy:
         if args.strategy[0].isupper():
@@ -602,7 +597,7 @@ def main_export(args):
     copytree = shutil.move if args.move else None
 
     project = get_project()
-    jobs = [project.open_job(id=job_id) for job_id in find_with_filter(args)]
+    jobs = [project.open_job(id=job_id) for job_id in _find_with_filter(args)]
 
     paths = {}
     with tqdm(total=len(jobs), desc="Export") as pbar:
@@ -664,27 +659,6 @@ def main_migrate(args):
         apply_migrations(root)
 
 
-def verify_config(cfg, preserve_errors=True):
-    """Verify provided configuration."""
-    verification = cfg.verify(preserve_errors=preserve_errors)
-    if verification is True:
-        _print_err("Passed.")
-    else:
-        for entry in flatten_errors(cfg, verification):
-            # each entry is a tuple
-            section_list, key, error = entry
-            if key is not None:
-                section_list.append(key)
-            else:
-                section_list.append("[missing section]")
-            section_string = ".".join(section_list)
-            if error is False:
-                error = "Possibly invalid or missing."
-            else:
-                error = type(error).__name__
-            _print_err(" ".join((section_string, ":", error)))
-
-
 def main_config_show(args):
     """Handle config show subcommand."""
     cfg = None
@@ -714,10 +688,31 @@ def main_config_show(args):
     if not isinstance(cfg, Section):
         print(cfg)
     else:
-        for line in config.Config(cfg).write():
+        for line in config._Config(cfg).write():
             print(line)
 
 
+def _verify_config(cfg, preserve_errors=True):
+    """Verify provided configuration."""
+    verification = cfg.verify(preserve_errors=preserve_errors)
+    if verification is True:
+        _print_err("Passed.")
+    else:
+        for entry in flatten_errors(cfg, verification):
+            # each entry is a tuple
+            section_list, key, error = entry
+            if key is not None:
+                section_list.append(key)
+            else:
+                section_list.append("[missing section]")
+            section_string = ".".join(section_list)
+            if error is False:
+                error = "Possibly invalid or missing."
+            else:
+                error = type(error).__name__
+            _print_err(" ".join((section_string, ":", error)))
+
+
 def main_config_verify(args):
     """Handle config verify subcommand."""
     cfg = None
@@ -742,7 +737,7 @@ def main_config_verify(args):
     else:
         if cfg.filename is not None:
             _print_err(f"Verification of config file '{cfg.filename}'.")
-        verify_config(cfg)
+        _verify_config(cfg)
 
 
 def main_config_set(args):
@@ -794,7 +789,7 @@ def main_shell(args):
         print("No project within this directory.")
         print("If you want to initialize a project, execute `$ signac init`.")
     else:
-        _jobs = find_with_filter(args)
+        _jobs = _find_with_filter(args)
 
         def jobs():
             for id_ in _jobs:
diff --git a/signac/common/config.py b/signac/common/config.py
index d4543a9b..18c67db8 100644
--- a/signac/common/config.py
+++ b/signac/common/config.py
@@ -97,10 +97,18 @@ def _locate_config_dir(search_path):
             search_path = up
 
 
+class _Config(ConfigObj):
+    """Manages configuration for a signac project."""
+
+    def verify(self, *, preserve_errors=False):
+        """Validate the contents of this configuration."""
+        return super().validate(Validator(), preserve_errors=preserve_errors)
+
+
 def _read_config_file(filename):
     logger.debug(f"Reading config file '{filename}'.")
     try:
-        config = Config(filename, configspec=_CFG.split("\n"))
+        config = _Config(filename, configspec=_CFG.split("\n"))
     except (OSError, ConfigObjError) as error:
         raise ConfigError(f"Failed to read configuration file '{filename}':\n{error}")
     verification = config.verify()
@@ -125,14 +133,14 @@ def _load_config(path=None):
 
     Returns
     --------
-    :class:`Config`
+    :class:`_Config`
         The composite configuration including both project-local and global
         config data if requested. Note that because this config is a composite,
         modifications to the returned value will not be reflected in the files.
     """
     if path is None:
         path = os.getcwd()
-    config = Config(configspec=_CFG.split("\n"))
+    config = _Config(configspec=_CFG.split("\n"))
 
     # Add in any global or user config files. For now this only finds user-specific
     # files, but it could be updated in the future to support e.g. system-wide config files.
@@ -143,15 +151,3 @@ def _load_config(path=None):
     if os.path.isfile(_get_project_config_fn(path)):
         config.merge(_read_config_file(_get_project_config_fn(path)))
     return config
-
-
-class Config(ConfigObj):
-    """Manages configuration for a signac project."""
-
-    encoding = "utf-8"
-
-    def verify(self, validator=None, *args, **kwargs):
-        """Validate the contents of this configuration."""
-        if validator is None:
-            validator = Validator()
-        return super().validate(validator, *args, **kwargs)
diff --git a/signac/contrib/migration/v1_to_v2.py b/signac/contrib/migration/v1_to_v2.py
index ac108eaf..3e5aa5ca 100644
--- a/signac/contrib/migration/v1_to_v2.py
+++ b/signac/contrib/migration/v1_to_v2.py
@@ -47,8 +47,6 @@ def _migrate_v1_to_v2(root_directory):
     """Migrate from schema version 1 to version 2."""
     # Load the v1 config.
     cfg = _load_config_v1(root_directory)
-    fn_doc = os.path.join(root_directory, Project.FN_DOCUMENT)
-    doc = BufferedJSONAttrDict(filename=fn_doc, write_concern=True)
 
     # Try to migrate a custom workspace directory if one exists.
     current_workspace_name = cfg.get("workspace_dir")
@@ -66,8 +64,12 @@ def _migrate_v1_to_v2(root_directory):
             os.replace(current_workspace, new_workspace)
         del cfg["workspace_dir"]
 
-    # Delete project name from config and store in project doc.
-    doc["signac_project_name"] = cfg["project"]
+    # Delete project name from config and store in project doc if non-default.
+    # For default names, no modifications to the project document should be made.
+    if cfg["project"] != "None":
+        fn_doc = os.path.join(root_directory, Project.FN_DOCUMENT)
+        doc = BufferedJSONAttrDict(filename=fn_doc, write_concern=True)
+        doc["signac_project_name"] = cfg["project"]
     del cfg["project"]
     cfg.write()
 
@@ -79,12 +81,10 @@ def _migrate_v1_to_v2(root_directory):
 
     # Now move all other files.
     files_to_move = {
-        ".signac_shell_history": os.sep.join((".signac", "shell_history")),
-        ".signac_sp_cache.json.gz": os.sep.join(
-            (".signac", "statepoint_cache.json.gz")
-        ),
+        ".signac_shell_history": os.path.join(".signac", "shell_history"),
+        ".signac_sp_cache.json.gz": os.path.join(".signac", "statepoint_cache.json.gz"),
     }
     for src, dst in files_to_move.items():
-        src = os.sep.join((root_directory, src))
+        src = os.path.join(root_directory, src)
         if os.path.isfile(src):
-            os.replace(src, os.sep.join((root_directory, dst)))
+            os.replace(src, os.path.join(root_directory, dst))
diff --git a/signac/contrib/project.py b/signac/contrib/project.py
index 7d44460d..ebbaad02 100644
--- a/signac/contrib/project.py
+++ b/signac/contrib/project.py
@@ -22,7 +22,7 @@ from threading import RLock
 
 from .._synced_collections.backends.collection_json import BufferedJSONAttrDict
 from ..common.config import (
-    Config,
+    _Config,
     _get_project_config_fn,
     _load_config,
     _locate_config_dir,
@@ -51,7 +51,7 @@ JOB_ID_LENGTH = 32
 JOB_ID_REGEX = re.compile(f"[a-f0-9]{{{JOB_ID_LENGTH}}}")
 
 
-class _ProjectConfig(Config):
+class _ProjectConfig(_Config):
     r"""Extends the project config to make it immutable.
 
     Parameters
@@ -232,7 +232,7 @@ class Project:
         elif config_schema_version < schema_version:
             raise IncompatibleSchemaVersion(
                 "The signac schema version used by this project is '{}', but signac {} "
-                "requires schema version '{}'. Please use '$ signac migrate' to "
+                "requires schema version '{}'. Please use 'python -m signac migrate' to "
                 "irreversibly migrate this project's schema to the supported "
                 "version.".format(config_schema_version, __version__, schema_version)
             )
diff --git a/signac/contrib/utility.py b/signac/contrib/utility.py
index a5601b8c..b8db8ea1 100644
--- a/signac/contrib/utility.py
+++ b/signac/contrib/utility.py
@@ -56,14 +56,13 @@ def _query_yes_no(question, default="yes"):  # pragma: no cover
         raise ValueError("invalid default answer: '%s'" % default)
 
     while True:
-        sys.stdout.write(question + prompt)
-        choice = input().lower()
+        choice = input(question + prompt).lower()
         if default is not None and choice == "":
             return valid[default]
         elif choice in valid:
             return valid[choice]
         else:
-            sys.stdout.write("Please respond with 'yes' or 'no' (or 'y' or 'n').\n")
+            print("Please respond with 'yes' or 'no' (or 'y' or 'n').")
 
 
 def _add_verbosity_argument(parser, default=0):


Test patch:
diff --git a/tests/test_searchindexer.py b/tests/test_searchindexer.py
index b611d59e..2470e2b1 100644
--- a/tests/test_searchindexer.py
+++ b/tests/test_searchindexer.py
@@ -219,16 +219,16 @@ class TestSearchIndexer:
         assert list(self.c.find({"a": {"$type": "float"}})) == ["float"]
         assert list(self.c.find({"a": {"$type": "int"}})) == ["int"]
 
-    # TODO: Document that dots are not allowed in keys of the document. This is
-    # a condition for correctness but is not enforced.
     def test_docs_with_dots(self):
+        # Dots are not allowed in keys of the document. Here, we explicitly
+        # test to ensure that an error is raised if invalid keys are found.
         self.c["0"] = {"a.b": 0}
 
-        # These searches will not catch the error:
+        # These searches will not trigger an error:
         self.c.find()
         self.c.find({"a": 0})
 
-        # This one will:
+        # This search triggers an error:
         with pytest.raises(InvalidKeyError):
             self.c.find({"a.b": 0})
 
diff --git a/tests/test_shell.py b/tests/test_shell.py
index efa70676..c675f6a0 100644
--- a/tests/test_shell.py
+++ b/tests/test_shell.py
@@ -744,17 +744,17 @@ class TestBasicShell:
         self.call("python -m signac init".split())
         out = self.call("python -m signac config --local show".split()).strip()
         cfg = config._read_config_file(".signac/config")
-        expected = config.Config(cfg).write()
+        expected = config._Config(cfg).write()
         assert out.split(os.linesep) == expected
 
         out = self.call("python -m signac config show".split()).strip()
         cfg = config._load_config()
-        expected = config.Config(cfg).write()
+        expected = config._Config(cfg).write()
         assert out.split(os.linesep) == expected
 
         out = self.call("python -m signac config --global show".split()).strip()
         cfg = config._read_config_file(config.USER_CONFIG_FN)
-        expected = config.Config(cfg).write()
+        expected = config._Config(cfg).write()
         assert out.split(os.linesep) == expected
 
     def test_config_set(self):