Include comments in config files generated with --create-config-file #588

SylviaWhittle · 2023-05-26T19:43:02Z

Closes #536

This PR changes the behaviour of generating config files using the command-line flag --create-config-file, so that it uses shutil to copy the default_config.yaml file directly. This is a trade-off on functionality, but it seems that users would prefer the convenience of having the comments in the file.

If it turns out that the users do not end up liking this change, it should be trivial to reverse it as it's a small change that does not affect any other code as far as I can tell.

…ith yaml

codecov-commenter · 2023-05-26T20:15:43Z

Codecov Report

Patch coverage: 78.26% and project coverage change: +0.08 🎉

Comparison is base (6590f15) 80.73% compared to head (78c6e18) 80.82%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #588      +/-   ##
==========================================
+ Coverage   80.73%   80.82%   +0.08%     
==========================================
  Files          19       19              
  Lines        2819     2832      +13     
==========================================
+ Hits         2276     2289      +13     
  Misses        543      543

Impacted Files	Coverage Δ
topostats/run_topostats.py	`83.72% <55.55%> (+0.90%)`	⬆️
topostats/io.py	`81.26% <92.85%> (+0.39%)`	⬆️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

ns-rse · 2023-05-26T22:28:26Z

topostats/run_topostats.py

-            config_file=args.create_config_file,
-            header_message="Sample configuration file auto-generated",
-        )
+        default_config_path = pkg_resources.path(__package__, "default_config.yaml")


I understand why this is here but I think we can make things a little simpler by changing some code earlier on.

Currently on lines 146-150 we have...

if args.config_file is not None: config = read_yaml(args.config_file) else: default_config = pkg_resources.open_text(__package__, "default_config.yaml") config = yaml.safe_load(default_config.read())

...and so we have two lines that are very similar here as they both have pkg_resources.open_text(__package__, "default_config.yaml").

If we change the logic of the earlier code we only need to have one line for setting the path to the default_config.yaml...

if args.config_file is None: default_config = pkg_resources.open_text(__package__, "default_config.yaml") config = yaml.safe_load(default_config.read()) else: config = read_yaml(args.config_file)

...and the above line is no longer needed (although line 172 would need changing to use default_config rather than default_config_path).

I could be misunderstanding you.

pkg_resources.open_text(__package__, "default_config.yaml")

returns a string of the contents of the config file, which is not usable by shutil.copy, which takes a file system path. Which is the reason for using

pkg_resources.path(__package__, "default_config.yaml")

This could work if I manually saved to the file using with open() as f, is this what you intended?

ns-rse · 2023-05-26T22:30:23Z

topostats/run_topostats.py

-        )
+        default_config_path = pkg_resources.path(__package__, "default_config.yaml")
+        if ".yaml" not in args.create_config_file and ".yml" not in args.create_config_file:
+            create_config_path = "./" + args.create_config_file + ".yaml"


Not tested this on M$-Win but would the hard coding of ./ cause any problems? I've a vague memory that the Windows Linux Shell does interpret/handle these correctly. If not perhaps using pathlib.Path("./") might mitigate the problem (if there is one!).

ns-rse

Thanks for getting this one sorted too @SylviaWhittle

I've suggested a slight change to avoid duplicated lines simply by changing earlier logic.

I think its a bit of a shame to lose the file header which indicates when the sample config was generated but date/time isn't too reliable given people could be on different branches anyway and besides we ask for the config used at any given point to be included in Bug Reports anyway.

…tputs

SylviaWhittle · 2023-05-27T00:25:09Z

Thanks for the comments @ns-rse, I've just pushed a re-jig of it, where I use your suggestion above, and I create a function in io.py, write_config_with_comments to handle the file writing.

I had to add a line to ensure users are not running with --create-config-file and --config, as this would leave default_config undefined before its call.

I've also moved things about a bit with the documentation, so that we keep the # Config file generated 2023-05-27 01:01:12 and documentation links in the config file. Yay!

There were some 404 links in the code that I have removed, leaving only a link that works. This should also mean that it should be easy to have all documentation hints standardised and removed link duplication in the codebase

Let me know what you think and I'll add tests if you approve of what I've attempted (don't worry I hadn't forgotten about adding tests). 👍

ns-rse

Neat, I like this solution of using a string object rather than shutil.copy and that you've retained the file header at the same time.

Not sure its absolutely necessary to use Path.cwd() may be overkill. 🤷

ns-rse · 2023-05-30T14:25:34Z

topostats/io.py

    if header_message:
        config_yaml.yaml_set_start_comment(
-            f"{header_message} : {datetime.now().strftime('%Y-%m-%d %H:%M:%S')}\n" + documentation_reference
+            f"{header_message} : {datetime.now().strftime('%Y-%m-%d %H:%M:%S')}\n" + CONFIG_DOCUMENTATION_REFERENCE


Could use the get_date_time() function here as you do below.

ns-rse · 2023-05-30T14:27:39Z

topostats/io.py

+        A name for the configuration file. Can have a ".yaml" on the end.
+    """
+
+    if ".yaml" not in filename and ".yml" not in filename:


Good to have this eventuality caught 👍

ns-rse · 2023-05-30T14:28:10Z

topostats/io.py

@@ -86,6 +103,32 @@ def write_yaml(
            LOGGER.error(exception)


+def write_config_with_comments(config: str, filename: str = "config.yaml"):


Minor pedantic typehint for the return value.

Suggested change

def write_config_with_comments(config: str, filename: str = "config.yaml"):

def write_config_with_comments(config: str, filename: str = "config.yaml") -> None:

ns-rse · 2023-05-30T14:31:13Z

topostats/io.py

+        f.write(f"# Config file generated {get_date_time()}\n")
+        f.write(f"# {CONFIG_DOCUMENTATION_REFERENCE}")
+        f.write(config)
+    LOGGER.info(f"A sample configuration has been written to : ./{filename}")


Suggested change

LOGGER.info(f"A sample configuration has been written to : ./{filename}")

LOGGER.info(f"A sample configuration has been written to : ./{str(create_config_path)}")

...although conditional on whether you use Path.cwd() / Path("./") / filename above. If not you don't need the str().

ns-rse · 2023-05-30T14:32:21Z

topostats/io.py

+    """
+
+    if ".yaml" not in filename and ".yml" not in filename:
+        create_config_path = "./" + filename + ".yaml"


Might be overkill using Path.cwd() here and under the else: but could.

Suggested change

create_config_path = "./" + filename + ".yaml"

create_config_path = Path.cwd() / f"{filename}.yaml"

ns-rse · 2023-05-30T14:33:38Z

topostats/run_topostats.py

@@ -162,19 +170,10 @@ def main(args=None):
    validate_config(config, schema=DEFAULT_CONFIG_SCHEMA, config_type="YAML configuration file")

    # Write sample configuration if asked to do so and exit
+    if args.create_config_file and args.config_file:


ns-rse · 2023-05-30T14:40:03Z

topostats/run_topostats.py

-    if args.config_file is not None:
-        config = read_yaml(args.config_file)
+    if args.config_file is None:
+        default_config = pkg_resources.open_text(__package__, "default_config.yaml").read()


Ah, very neat trick of shifting the .read() to get the string representation of the file.

ns-rse

Looks good to go when the tests complete. 👍

Copy default_config.yaml using shutil instead of loading and saving w…

c8f7ad8

…ith yaml

ns-rse reviewed May 26, 2023

View reviewed changes

Move config string saving into io.py and standardise documentation ou…

b7a4cc6

…tputs

SylviaWhittle requested a review from ns-rse May 30, 2023 11:01

ns-rse approved these changes May 30, 2023

View reviewed changes

SylviaWhittle added 2 commits May 30, 2023 16:37

PR feedback, thanks @ns-rse

f678918

Add tests for added functions in io.py

78c6e18

ns-rse approved these changes May 30, 2023

View reviewed changes

SylviaWhittle added this pull request to the merge queue May 30, 2023

Merged via the queue into main with commit 615b03d May 30, 2023

SylviaWhittle deleted the SylviaWhittle/536-config-file-comments branch June 21, 2023 12:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Include comments in config files generated with --create-config-file #588

Include comments in config files generated with --create-config-file #588

SylviaWhittle commented May 26, 2023

codecov-commenter commented May 26, 2023 •

edited

Loading

ns-rse May 26, 2023

SylviaWhittle May 27, 2023 •

edited

Loading

ns-rse May 26, 2023

ns-rse left a comment

SylviaWhittle commented May 27, 2023 •

edited

Loading

ns-rse left a comment

ns-rse May 30, 2023

ns-rse May 30, 2023 •

edited

Loading

ns-rse May 30, 2023

ns-rse May 30, 2023

ns-rse May 30, 2023

ns-rse May 30, 2023

ns-rse May 30, 2023

ns-rse left a comment

		@@ -86,6 +103,32 @@ def write_yaml(
		LOGGER.error(exception)


		def write_config_with_comments(config: str, filename: str = "config.yaml"):

	LOGGER.info(f"A sample configuration has been written to : ./{filename}")
	LOGGER.info(f"A sample configuration has been written to : ./{str(create_config_path)}")

	create_config_path = "./" + filename + ".yaml"
	create_config_path = Path.cwd() / f"{filename}.yaml"

Include comments in config files generated with --create-config-file #588

Include comments in config files generated with --create-config-file #588

Conversation

SylviaWhittle commented May 26, 2023

codecov-commenter commented May 26, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

SylviaWhittle May 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ns-rse left a comment

Choose a reason for hiding this comment

SylviaWhittle commented May 27, 2023 • edited Loading

ns-rse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ns-rse May 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ns-rse left a comment

Choose a reason for hiding this comment

codecov-commenter commented May 26, 2023 •

edited

Loading

SylviaWhittle May 27, 2023 •

edited

Loading

SylviaWhittle commented May 27, 2023 •

edited

Loading

ns-rse May 30, 2023 •

edited

Loading