Masterview devel - Sep 2006 - Code review question/comment - MV module constant to access the config settings

Jeff - hey!  I''m alive again!  Finally crawling out of the swamp of
patching-bad-code I''ve been in for the past two months.  Back to ruby
and
masterview at last!!

Synched all your changes from the past month, the new directives stuff looks
*much* better.  I''ll poke around a bit and maybe have some additional
impressions after a bit more experimenting and getting back to my effort to
document creating directives, but overall this is very nice.  Good job.

Minor question/comment: in the initialization logic, you added creation of a
new constant MasterView::LoadedConfiguration.  But... we already have that,
so either this is unnecessary duplication or there''s something going on
in
startup ordering that I missed and should revisit to fix correctly.

The initializer basically goes through 3 phases:

(1) initialize_configuration - load the config, picking up user settings,
run some validation checks to ensure goodness, normalize representations
(ensure file system root paths are abs refs), and record values so config is
available to the runtime system

(2) load_plugin - load masterview itself (require all the core code)

(3) complete_plugin_installation - session startup: init logger and MIO,
load directives, do all our init for parsing, change detection, the admin
pages, etc

The last thing done in initialize_configuration is:

    MasterView.const_set(''ConfigSettings'', configuration)

with the intent that MasterView::ConfigSettings be available during
operation for code which needed to access config-dependent values.

So I think your addition of LoadedConfiguration is unnecessary, unless
there''s something subtle about start order that I''ve missed in
the places
where you''re referencing that new guy. 

~ Deb

On 9/7/06, Deb Lewis <djlewis at acm.org> wrote:
> Jeff - hey!  I''m alive again!  Finally crawling out of the swamp
of
> patching-bad-code I''ve been in for the past two months.  Back to
ruby and
> masterview at last!!
Wonderful! I''ve missed you :-)
>
>
> Minor question/comment: in the initialization logic, you added creation of
a
> new constant MasterView::LoadedConfiguration.  But... we already have that,
> so either this is unnecessary duplication or there''s something
going on in
> startup ordering that I missed and should revisit to fix correctly.
>
> The initializer basically goes through 3 phases:
>
> (1) initialize_configuration - load the config, picking up user settings,
> run some validation checks to ensure goodness, normalize representations
> (ensure file system root paths are abs refs), and record values so config
is
> available to the runtime system
>
> (2) load_plugin - load masterview itself (require all the core code)
>
> (3) complete_plugin_installation - session startup: init logger and MIO,
> load directives, do all our init for parsing, change detection, the admin
> pages, etc
>
> The last thing done in initialize_configuration is:
>
>     MasterView.const_set(''ConfigSettings'', configuration)
>
> with the intent that MasterView::ConfigSettings be available during
Yeah, maybe I missed the fact that the configuration was being set in
that constant and created some duplication. I can''t quite remember off
the top of my head (been in a lot of code recently :-). The only other
thing I might have been doing was trying to get the configuration that
ended up being used (which is the merge of default plus user
settings), not sure if that might have been what I was going after (if
ConfigSettings is only what the user had specified). I''ll take a look
tomorrow and see if anything rings a bell or whether I simply missed
the other constant.

Thanks for clarifying the load phases though, I didn''t quite have my
hands around how all that worked yet.

Jeff

>> ... trying to get the configuration that ended up being used 
>> (which is the merge of default plus user settings), not sure if 
>> that might have been what I was going after (if ConfigSettings 
>> is only what the user had specified). I''ll take a look
tomorrow
>> and see if anything rings a bell or whether I simply missed the 
>> other constant.
MasterView::ConfigSetting is "the true story" - that''s the
initial defaults
+ the user''s settings (possibly environment-dependent) + any final
cleanup/normalizing we do.  So it''s definition is "the actual,
final
configuration settings for this application execution session".

I decided to be a bit aggressive and actually freeze this configuration
instance at the time we install it in the module constant so there''s no
possible question about what config we were supposed to be using.  Maybe
overzealous, but then it''s also a clear distinction between a config
setting
which initializes some variable which can change over the life of the
application and something which is truly intended to be a constant once
it''s
value has been determined.

We could also go back at this point and drop out some of the original
constants you''d used in various internal classes to capture this sort
of
stuff, at least some of them are probably redundant now.  Places where we
stuff a config value into an internal implementation class''s constant
could
be dropped out s.t. the class gets its value directly from a
MasterView::ConfigSettings.foobar value.  I didn''t want to do that
originally because I was nervous about making too many changes during the
transition to the new config scheme.

I decided *not* to document the existence of MasterView::ConfigSettings in
the Configuration documentation - I wanted that to be user-focused and about
the specification mechanism ("here''s how you specify your config
options"),
whereas the config instance recording the settings is primarily an internal
artifact of the MV implementation and intended for use by the engine itself
moreso that client apps.

Would be more appropriate to document this guy in a docs section on the MV
internals, I think.  I''ll see if it fits in with the docs I''ve
started to
describe the directive implementation technique, if that works as a general
home for "Extending MasterView" material.

~ Deb

> On 9/7/06, Deb Lewis <djlewis at acm.org> wrote:
>
> > Minor question/comment: in the initialization logic, you added
creation of a
> > new constant MasterView::LoadedConfiguration.  But... we already have
that,
> > so either this is unnecessary duplication or there''s
something going on in
> > startup ordering that I missed and should revisit to fix correctly.
> >
> >
> > The last thing done in initialize_configuration is:
> >
> >     MasterView.const_set(''ConfigSettings'',
configuration)
> >
> > with the intent that MasterView::ConfigSettings be available during
>
Yes, it appears that I simply needed access to the configuration from
the masterview_controller and for the enable_mv_admin_page
functionality. So it does appear to be a duplication. I will change
the references of LoadedConfiguration to ConfigSettings and make sure
that everything still works properly, then we can delete the
duplicate.

Nice catch!

Jeff

On 9/8/06, Deb Lewis <djlewis at acm.org> wrote:
>
> MasterView::ConfigSetting is "the true story" - that''s
the initial defaults
> + the user''s settings (possibly environment-dependent) + any final
> cleanup/normalizing we do.  So it''s definition is "the
actual, final
> configuration settings for this application execution session".
Great, that the guy I would use most often.
>
> Would be more appropriate to document this guy in a docs section on the MV
> internals, I think.  I''ll see if it fits in with the docs
I''ve started to
> describe the directive implementation technique, if that works as a general
> home for "Extending MasterView" material.
>
I think that would be a good place to document it, keeping the other
stuff more user oriented like you said.