Change config filenames #46
Conversation
for more information, see https://pre-commit.ci
FabioRosado
left a comment
FabioRosado
left a comment
There was a problem hiding this comment.
Looks good, added a small comment (more like a though haha)
| manifest_file = app_dir / config["project_config_filename"] | ||
| with manifest_file.open("w", encoding="utf-8") as fp: | ||
| json.dump(context, fp) | ||
| if str(manifest_file).endswith(".json"): |
There was a problem hiding this comment.
Just wondering, what do you think about using fp.name instead of converting manifest_file to a string?
I guess manifest_file is more readable tho (unless we change the fp to something haha)
There was a problem hiding this comment.
Good point... I think I'd lean more toward what you said in terms of readability...
There was a problem hiding this comment.
There is a method on Path, .suffix, that you might consider using instead of using string operations.
| APPNAME = "pyscript" | ||
| APPAUTHOR = "python" | ||
| DEFAULT_CONFIG_FILENAME = "pyscript.json" | ||
| DEFAULT_CONFIG_FILENAME = ".pyscriptconfig" |
There was a problem hiding this comment.
Maybe a comment would help here to clear up the difference between the config file for the CLI itself, and the project config file? When I first looked at the code it took me a minute to see why DEFAULT_CONFIG_FILENAME didn't match the value in DEFAULT_CONFIG. Or maybe (although it is longer), always refer to them as either CLI_CONFIG_FILENAME, cli_config and PROJECT_... etc etc?
| manifest_file = app_dir / config["project_config_filename"] | ||
| with manifest_file.open("w", encoding="utf-8") as fp: | ||
| json.dump(context, fp) | ||
| if str(manifest_file).endswith(".json"): |
There was a problem hiding this comment.
Naming nit? The constant is called "project_config_filename" but then we use "manifest_file" for the path? Maybe "project_config_path = ..."?
| with manifest_path.open() as fp: | ||
| contents = json.load(fp) | ||
| # with a default config path | ||
| manifest_path = tmp_cwd / app_name / config["project_config_filename"] |
There was a problem hiding this comment.
Ditto for previous comment about picking one name.
Close #39
Basically:
pyscript.json-->.pyscriptconfigmanifest.json-->pyscript.tomljsonortoml