Closed mikkoi closed 2 years ago
I started reviewing this and stopped because it's a bit difficult. There are movements done between files with some renaming and I need to keep going back-and-forth to see what was moved, what was altered, etc. There is no explanation in the commit. I started with comments, then saw this was the original code copied, etc. I would have preferred this to be done either with several commits - each accomplishing one clear thing, or with a description with the commits of what they're doing. (Commits that both move code and change the code are some of the most difficult to properly review and that's when multiple commits are really useful.)
I'll start with a few small comments and try to review it more thoroughly meanwhile.
FileSimple
. Other options: File::Simple
, Simple::File
, or just File
.Here is partial coverage of what this PR does:
Role::Config
which takes away most of Role::ConfigReader
Role::ConfigReader
: default_config
, config
, config_files
, local_triggers
, and global_triggers
.config_location
, environments_location
, environment
, and config_readers
.Role::ConfigReader
: _build_default_config
, _build_environment
, _build_config_files
, _build_config
, _set_config_entries
, _set_config_entry
, _normalize_config
, _compile_config
, settings
, setting
, has_setting
, $_normalizers
, _normalize_config_entry
, and _compile_config_entry
._build_config_readers
Role::ConfigReader
, it leaves it with environment
and location
FileSimple
which uses Role::ConfigReader
and has a method to read the config.This feels like a bit of a maze and I'm still making sense of it. A map would be useful. :)
Thanks for the summarisation there @xsawyerx - definitely makes it easier for other reviewers, kudos.
In general, it seems like a good idea allowing much more flexibility in config reading - thanks @mikkoi for your work!
I'll try to review in more detail as soon as I get some free time.
I think it could do with a bit more documentation adding - i.e. documenting in the new D2::Core::ConfigReader::FileSimple that it is the original file-based config reader, and briefly documenting in D2::Core::Role::ConfigReader how to implement custom config readers based on it.
My understanding from a quick review is that the basic idea is for D2::Core::Role::Config
to handle the "how to get config" stuff, and for D2::Core::Role::ConfigReader
to be used by config readers, e.g. Dancer2::ConfigReader::FileSimple
by default, to handle the actual reading of a config from a source. That seems like a pretty sane design.
I agree, it confuses when one commit has both file renames and changes. I created a new PR. Separated the commits and improved the docs a little. https://github.com/PerlDancer/Dancer2/pull/1637
Also removed "Ye Olde Config Reader." and renamed FileSimple to File::Simple.
I am wondering if I should have named Dancer2::Core::Role::ConfigReader
something else. Now it confuses. Maybe ConfigBuilder
, ConfigAssembler
or ConfigCreator
?
I appreciate the new PR very much. I think we can close this one in favor of that. I was able to review it, provide comments, and a summary of what I think we should do next.
Development moved to https://github.com/PerlDancer/Dancer2/pull/1637
Completely new design for the config system.
Config are loaded by classes which implement the role
Dancer2::Core::Role::ConfigReader
. User can create custom classes to use instead ofDancer2::ConfigReader::FileSimple
. Custom classes are activated via env varDANCER_CONFIG_READERS
.