Validate config.yaml contents (#77)

[chelseadole]

Addresses #77

• Adds desert-based dataclasses to configuration.py to deserialize config.yaml contents
• Changes all references of dictionary bracket notation --> dataclass dot notation
• Adds tests

In the discussion for #77 , @jkbecker mentioned potentially making config-related changes discussed in #84 . I did not get to this, because the PR reached a critical length where I think it's safest to separate concerns. :)

[chelseadole]

RE: failing tests -- tests seem to be failing because the ci.yml builds the plotman package using dist/wheel rather than cloning, so config.yaml is not present.

Is this desired? Checking out the setup.cfg, I see:

[options.data_files]
config = config.yaml

but I don't see anything about config.yaml in the MANIFEST.in. What is the desired behavior RE: config.yaml being present when a user installs using pip?

For these tests, it would be possible to mock out the yaml.load(), so that it's not actually necessary for config.yaml to be present. However, IMO that somewhat defeats the purpose of the main test -- as it would be helpful to check the validity of the default/example config.yaml in this repo without expecting contributors to know they have to update a test-only mirror when adding new config values. Thoughts?

[jkbecker]

Awesome!

RE: failing tests -- tests seem to be failing because the ci.yml builds the plotman package using dist/wheel rather than cloning, so config.yaml is not present.

Haha, no, this is not desired. But also not a surprise.

When I proposed #61, we had an at-length discussion there about the config file being treated as a separate issue because it's just an intermediary weirdness and existing users expect it to sit in their current working directory anyways. The actual issue for taking care of config is #84 and I have not gotten around to implementing it. In fact I haven't touched it at all - if you're in the zone and this seems like a good fit, feel free to look into it, ... otherwise maybe @altendky has a good idea of how to hotfix the CI for now (he introduced it, it's mostly black magic to me) to make this one work in isolation?

[altendky]

Several specific comments and some general questions, but overall this is having exactly the effect I'd hoped for. Simple and complete definition of the configuration along with explicit errors for users rather than random code giving TypeError or such. :] Thanks for all the work on this. I have a few followup changes that I want to do that will be much more sensible because of this improvement.

1. For all (@jkbecker, @ericaltendorf), do we want to use attrs or dataclasses? https://www.attrs.org/en/stable/why.html#data-classes gives a brief explanation. I default to attrs. It shouldn't make much of a difference to the PR.

2. Yeah, we need to decide how to handle the config file. I haven't really made out a whole plan, but I'll write some stuff and see where we end up. We want to be able to, after installation, write out a file to a given location with a 'default' configuration. The content could be sourced from an existing file or from a function that creates instances of the new configuration classes. If it is a file, then yes, it should be validated in the tests as @chelseadole pointed out. I suppose it is pretty handy to have a file to link to in the repository and talk about. Maybe that's enough to make up my mind on using a file. I don't super care where it lives but I might default to something like src/plotman/resources/config.yaml (or plotman.yaml). https://github.com/ericaltendorf/plotman/pull/93/files provides a reference for including data files and using importlib.resources. Tests can get at the data and the runtime can get to it as well to write out the default config for the user.

I expect the options.data_files will go away. I think MANIFEST.in and include_package_data = True in setup.cfg will cover the data files we need.

Thanks again for making this big improvement here. The list of comments looks unrepresentatively long :[ but I think figuring out the config file location etc might be the biggest thing, and it shouldn't be too bad... :]

[chelseadole]

@altendky @jkbecker

Thank you both for the review, comments, and context. To unblock myself in this PR, I will change config.yaml to be src/plotman/resources/plotman.yaml (I like the rename suggestion to plotman.yaml), and will add to MANIFEST, etc so it's packaged correctly.

We want to be able to, after installation, write out a file to a given location with a 'default' configuration.

My personal approach would be adding new args similar to what's suggested in #84, so when you pip install plotman, you're able to run something like: plotman config generate, which would create a config file at a pre-set location on your computer based on appdirs. Assuming we include a default plotman.yaml in the package as discussed above, we can just copy that file over. With that set up, we'd be able to run plotman config path, etc as suggested by @jkbecker here

There's a lot more detail discussed in #84, and I don't want to overcrowd this PR. How about I do the above work to unblock myself with testing, and leave additional commandline features with plotman config <...> to a future PRs?

[altendky]

Agreed that the various discussions about finding the config file should be addressed in a separate PR.

[chelseadole]

@altendky Let me know what you think of the added changes. It now supports:

$ plotman config show
$ plotman config generate

For now, it only performs generate with a default location (no configurable location/path) but that could be a future improvement. I also added the default resources/plotman.yaml file to the package as discussed above

[jkbecker]

Are these two sections still correct? They seem to be superseded by what the generate command does?

[jkbecker]

This looks awesome! I feel like it's probably clean to stick with just one default config location in this PR, and address the overloadable config file situation mentioned in #84 in a separate follow-up PR (I'm happy to work on that one but I'm currently super maxed out with "work" work, so... not mad if you get to tackling it first - we can play by ear.)

[chelseadole]

This looks awesome! I feel like it's probably clean to stick with just one default config location in this PR, and address the overloadable config file situation mentioned in #84 in a separate follow-up PR (I'm happy to work on that one but I'm currently super maxed out with "work" work, so... not mad if you get to tackling it first - we can play by ear.)

Yes, I wanted to start with a constant config location before adding complexity. I'm also ramping up on a new project at "work" work, so I'll see -- but if I have time I'll try to tackle it!

[altendky]

Thanks for keeping at this. :]

[altendky]

As noted over in #107 (comment), I would rather keep at least the file object creation outside of this function, as it was before. This keeps a bit flatter structure and leaves control of at least the I/O mechanism at a higher level leaving more flexibility. For example, this would avoid the need for mocking to test this function. Do you have a reason that you prefer the present form instead that I should think about?

Wherever this lives...

Suggested change

	except FileNotFoundError:
	raise ConfigurationException(
	f"No 'plotman.yaml' file exists at expected location: '{config_file_path}'. To generate "
	f"default config file, run: 'plotman config generate'"
	)
	except FileNotFoundError as e:
	raise ConfigurationException(
	f"No 'plotman.yaml' file exists at expected location: '{config_file_path}'. To generate "
	f"default config file, run: 'plotman config generate'"
	) from e

[altendky]

Thanks @chelseadole, definitely glad to see this change. Sorry for the wait.