Re: Gallery's K3 class name/dir/file conventions (was "Capitalization")

As the alternative to "G3" as the subdir name, howzabout "Hook"?  Seems to
be a term on par with Controller and Model...

Shad

Sent from my Swiss Army Phone
On Mar 23, 2013 11:21 PM, "Shad Laws" <shad-xpYdmXCiSuZWk0Htik3J/[email protected]> wrote:

> Hey Bharat,
>
> Likewise, responding inline (and snipping a bit)...
>
> On 23 March 2013 00:58, Bharat Mediratta <[email protected]> wrote:
>
>>
>> I agree with lots of what I see here, so I'm going to address a few
>> specific things inline.
>>
>>
>>>  2) The standard approach isn't O(n^2) unless I'm severely
>>>> misunderstanding - if you try to load a class like Controller_Admin_Tags
>>>> it'd expand to modules/*/classes/Controller/Admin/Tags.php so it'd do N
>>>> file_exists calls, not N^2. It should be the same for TagEvent, etc.
>>>>
>>>
>>> You're right - to find TagEvent, it's O(n).  However, Gallery events are
>>> run by finding the event files of *all* modules, and searching for each of
>>> them in each directory, which is O(n^2).
>>>
>>> (Sidenote: writing a module-name-sensitive fild_file(), e.g. one that
>>> looks only in "tag" for "TagEvent," removes the possibility of it being
>>> extended by another module)
>>>
>>
>> Ah ok I see what you mean - it's N find_file() calls across M modules to
>> load the event class.  And if the event handler doesn't exist, then it's a
>> little heavier.  Kohana really should optimize this case away with a cache,
>> although we're also getting a benefit from the filesystem cache.  The
>> second round of these file_exists() calls should never touch disk.
>>
>> I agree that this *might* be bad, but if we're going to complicate the
>> codebase to account for it somebody has to run benchmarks first.  At this
>> stage, we're better off aiming for simplicity and optimizing later if and
>> when we find out that it's a real issue.  Before we release, we can move
>> stuff around as much as we want.
>>
>
> Hmm, good point.  If we're going to replace something, we should benchmark
> first to prove it's worth the trouble.  That's a good argument for making
> our system work as-is with Kohana's built-in functions.
>
> Speaking of good points, I went back and looked at Kohana::find_file()
> again.  I seemed to be under the impression that it only cached *found*
> paths, but upon second read, that doesn't appear to be true.  That's yet
> another reason why working around it probably wouldn't make a huge
> difference.  It turns out the authors of Kohana have spent more time than I
> have thinking about this... :-)
>
>
> - Being opinionated for the naming of REST API resources seems important,
>>> but something about enforcing this everywhere else doesn't sit right.  I
>>> feel like we shouldn't *have* to be opinionated here.
>>>
>>
>> Let me turn that around - why would we *not* be opinionated wherever
>> it's useful?  There's value in saying "Oh, the event handlers are all in
>> modules/foo/classes/FooEvent.php" - the simplicity in having only one way
>> to do things greatly reduces cognitive load.
>>
>
> If we're sticking with Kohana's functions and not making a wildcard-savvy
> loader, then this conversation is kinda moot, but for fun...
>
> <pseudo-philosophical tangent>
> I feel like there are three different cases here:
> 1. Not require a rule. (make the wildcard-savvy loader)
> 2. Require a rule. (be opinionated and enforce one way to do it)
> 3. Require a rule and make you define it. (extra line in module.info)
>
> While there are certainly arguments why 2 is better than 3, in the general
> case, it seems like 1 could often be better than 2, no?
> </pseudo-philosophical tangent>
>
>  The solution I proposed yesterday addresses these, but obviously does so
>>> at a cost.  Here's another possibility:
>>> - Put all of our special files into a separate subdir or two in classes
>>> (I added a "Rest" subdir, but it could be dumped into the main "G3" too).
>>> - Allow the files in "G3" to have any prefix they want (TagEvent,
>>> TAGEvent, or TagFooEvent all work).
>>> - Write our own routine to find and run event/theme/etc files.  It
>>> essentially runs a wildcard version of Kohana::find_file() to look for
>>> classes/G3/*Event.php, caches the result, then requires them (as needed -
>>> e.g. usually not all needed for Block).  This does mean that we're
>>> kinda/sorta implementing our own find_file() function, but after staring at
>>> it for awhile, I don't think it'd be too bad.  And, I think it'd be worth
>>> the payoff, especially considering the frequency with which we call event
>>> and theme functions.
>>>
>>> This means:
>>> - All classes stay in the classes dir and are strictly PSR-0 compliant.
>>> - The classes play nice with everything else in the classes directory,
>>> and can still override each other (e.g. class G3_FooEvent extends
>>> Tag_G3_TagEvent).
>>> - We still separate generic stuff from Gallery-specific stuff.
>>> - They can have the prefix TagAlbums, Tag, or PDF as devs wish without
>>> having to specify it or follow rules that specify it for them.
>>> - We can load the files ourselves using our own, optimized routine.
>>>
>>
>> I'm a little mixed on this.  Some thoughts:
>> - I can't tell if I like the separate subdir or not.  Frankly, that
>> implies that there's something to separate from, but many modules can do
>> everything they need to do with just and event handler and a block, etc.
>>
>
> Personally, I do.  To me, it has a similar significance to Controller or
> Model, in the sense that the names of their functions have special meanings
> outside of the class itself.  This is not the case, for example, for the
> Tag.php helper, which is why it shouldn't be in any subdir.
>
>
>
>> - We avoid using "G3" in the code.  This is a holdover from Gallery 2
>> which had a bit of an identity crisis in the code.  It's always "Gallery"
>> with no version number.
>>
>
> Ah, gotcha.  We can find something else.  I picked it mainly because it
> was short :-)
>
>
>
>> - I don't like the fact that files in the G3 dir can have any prefix -
>> people will invariably do stupid stuff with that.  There's no need for that
>> flexibility, IMO
>>
> - I don't want to write our own find_file if we can avoid it - it'd be
>> nice if anything we did to optimize things could be pushed upstream
>>
>
> These disappear if we just use the Kohana loaders, as they don't allow
> wildcards.
>
>
>
>>  - If we don't care about overloading these files, we can avoid using
>> find_file altogether, for example:
>>
>> foreach (Module::active() as $module) {
>>   $class = "{$module->name}Event";
>>   if (!class_exists($class)) {
>>     $event_file = sprintf("modules/%s/classes/%sEvent.php",
>> $module->name, ucfirst($module->name));
>>     if (file_exists($event_file)) {
>>       require_once($event_file);
>>     }
>>   }
>>   if (class_exists($class) && method_exists($class, $function)) {
>>     call_user_func_array(array($class, $function), $args);
>>   }
>> }
>>
>> That's a very straightforward adaptation of what we're doing for K2.  I
>> don't recommend it though - not until we determine that it's an actual
>> performance hit to use the K3 approach.
>>
>
> Aye, I agree.  In fact, even simply keeping a local list of which modules
> do and don't have a certain type of file (Event, Theme, etc) would be
> largely duplicating what find_file() already does.
>
>
> I hear what you're saying, but I'm not swayed by the performance or
>> opinion arguments.  I could be swayed towards a less opinionated stance,
>> but only if we can find some use cases where that flexibility really adds
>> value...
>>
>
> I think by now I'm convinced of all points you just made, with the sole
> exception being the separate directory for Gallery-specific stuff.  I think
> it gives a meaningful distinction between it and the other stuff.  Also,
> it'd give us the added benefit of being able to change our mind at a later
> date should someone decide to build an even-more-optimized,
> wildcard-capable loader.
>
> On a semi-related note, how do we feel about underscores in "views" names?
>  I like the idea of adding a teeny bit of hierarchy like you did with the
> Tag example (certainly no more than 1 level), and that removes a decent
> number of them, but not all.  Example: is
> /modules/gallery/views/admin/advanced_settings.php fine?  My vote is yes,
> personally...
>
> Take care,
> Shad
>
>
>
>>
>> -Bharat
>>
>>>
>

------------------------------------------------------------------------------
Everyone hates slow websites. So do we.
Make your web apps faster with AppDynamics
Download AppDynamics Lite for free today:
http://p.sf.net/sfu/appdyn_d2d_mar

__[ g a l l e r y - d e v e l ]_________________________

[ list info/archive --> http://gallery.sf.net/lists.php ]
[ gallery info/FAQ/download --> http://gallery.sf.net ]