Separate presets, construction in make_data_sets
#12
Conversation
The definitions of the actual preset configurations and the code that instantiated `DataSet` objects with those configurations were previously one and the same, with a lot of repetitive boilerplate. This attempts to separate out the actual preset configurations from the code that creates DataSets, and in so doing make the module a little easier to read and to modify. Presets are now stored in a dict named `PRESETS` and `make_data_sets()` does nothing beyond picking the appropriate presets and sending them as arguments to the `DataSet` constructor.
| # "SDWA Return to Compliance": dict( | ||
| # echo_type="SDWA", | ||
| # table_name="SDWA_RETURN_TO_COMPLIANCE", | ||
| # idx_field="PWSID", | ||
| # date_field="FISCAL_YEAR", | ||
| # date_format="%Y" | ||
| # ) |
There was a problem hiding this comment.
This preset was there before but commented out. Normally I’d just drop stuff like that, but opted to keep it here (but reformatted) in case this was only temporarily commented out. LMK if it should just be removed.
There was a problem hiding this comment.
Let's keep it, if only as a note to ourselves that this table exists in the database and that this is its configuration.
I removed some notes to myself
There was a problem hiding this comment.
This is amazing! Thank you @Mr0grog!!! I agree it improves readability considerably. I add a few small minor edits to remove comments (notes to myself).
The downside here is that you can’t as easily do any complicated logic when creating a DataSet (since this set-up inherently treats every configuration the same way), but that’s not something that was being done before, and I’m not sure if it’s even something you want to be easy.
Yea, from a public user-facing perspective, I think we want control over how the DataSet objects are constructed and these presets let us do that. We don't want people with minimal understanding of the database to be setting/using the wrong date field or way of summarizing violations.
However, from an internal, research perspective, I can see how it may be useful to be able to reconfigure the presets from time to time. That said, it's never really come up and we can do that in different ways anyway.
I'm just commenting, to see if @shansen5 has a chance to weigh in, but feel free to merge after 3 days...
The definitions of the actual preset configurations and the code that created
DataSetobjects with those configurations were previously one and the same, with a lot of repetitive boilerplate. This attempts to separate out the actual preset configurations from the code that creates DataSets, and in so doing make the module a little easier to read and to modify.Presets are now stored in a dict named
PRESETSandmake_data_sets()does nothing beyond picking the appropriate presets and sending them as arguments to theDataSetconstructor.iflogic and other code that used to be between the presets also makes it a bit easier to see the names and the values that are associated with them.The downside here is that you can’t as easily do any complicated logic when creating a DataSet (since this set-up inherently treats every configuration the same way), but that’s not something that was being done before, and I’m not sure if it’s even something you want to be easy.
If you don’t feel like this meaningfully improves the readability and editability of this module, please throw out this PR! It’s a big change, but doesn’t affect functionality at all.