[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [GT] SVN Commit r623 - trunk/GT
Weigert, Thomas wrote:
Lets keep in mind what the original situation was:
Whenever an object alias is required, the following files were loaded:
'/usr/share/geniustrader/aliases/signals'
'/usr/share/geniustrader/aliases/indicators'
'/usr/share/geniustrader/aliases/systems'
'/usr/share/geniustrader/aliases/closestrategy'
'/usr/share/geniustrader/aliases/moneymanagement'
'/usr/share/geniustrader/aliases/tradefilters'
'/usr/share/geniustrader/aliases/orderfactory'
'/usr/share/geniustrader/aliases/analyzers'
This is bad for the following reasons:
1. The user has no control where these directories were. If one wanted
to use object aliases that are shared, one had to create these
directories. A user may not be allowed to create subdirectories in
/usr/share
hold on there sparky -- the users $HOME/.gt/aliases/$kind dirs
as well as GT::Conf::get("Path::Aliases::$kind" dirs were also
always searched so users had multiple places for their aliases.
sharing would suggest group need so some one would be granted
the requisite privileges necessary to create/maintain them.
but i agree that /usr/share/geniustrader/aliases in gt is
something of an anomaly.
2. These files are loaded every time an object alias is resolved, albeit
their content is needed only once.
yep -- bad implementation
You are correct in that loading these (if defined) in the loading of
options did not happen before, but
1. now it is well controlled and can also be prevented
2. the directories can be chosen to a users liking.
the only difference i see is no default values for
GT::Conf::get("Path::Aliases::$kind".
and since GT::Conf::get("Path::Aliases::$kind" is searched
last taht allowed a users alias to be altered by one from shared.
but all this is fine as far as i'm concerned. my objections
are elsewhere -- see comments at the bottom.
-----Original Message-----
From: Robert A. Schmied [mailto:ras
AT
acm.org]
Sent: Wednesday, May 07, 2008 4:43 PM
To: devel
AT
geniustrader.org
Subject: Re: [GT] SVN Commit r623 - trunk/GT
taking this a bit further, if an app (script) were to be developed
that
expressly wanted to load a file other than $HOME/.gt/options from
someplace
other than $HOME/.gt, by extension i'd expect it might want to get
alias
files from that other someplace as well. this change doesn't support
that
altered directory path.
Well, I might be misunderstanding you, but if a user would load from
other than the $HOME/.gt location, whatever file is loaded can define
Path::Aliases::<object kind> differently and thus cater to your
requirement.
even though it requires another method, say load_object_aliases() and
a call to it added (some how) in every script (application) i think
that would be the preferred solution.
Don't see the point of adding more burden for the user.
I must be misunderstanding your need above. Can you please outline the
requirements?
Note that in the previous scheme, none of what you are talking about
here would be possible.
an alternative, one that doesn't support an alternate directory (other
than the default GT::Conf::_get_home_path()."/.gt/aliases/", would be
a semaphore on GT::Tools::resolve_object_alias that inhibits multiple
loads. i think what would be simpler and have the same intent.
Yes, but what would the point be?
ps -- GT::Tools::resolve_object_alias (isn't) wasn't called in the
unchanged version unless an object alias is detected via the "@"
marker.
the "@" marker proposed removal would require
GT::Tools::resolve_object_alias
to be called for every call to create_standard_object.
You are correct. There would be an additional call in each case. This is
not a big overhead, as those objects are not created in a tight loop.
I actually don't care one way or the other. You raised the question
about what object aliases were, and I went to look into the detail and
found this inconsistency.
System aliases are called just like normal names, object aliases require
"@". If users are not bothered by that, no need to change.
also note that GT::ArgsTree::parse_args (at least the ones i've got)
resolve object aliases using the "@" marker as well, that wasn't
changed
in the proposed object_alias.patch
You are correct. Thanks.
As said earlier, I am fine with leaving things as is... I am not using
object aliases...
some how we are miss-communicating here a bit i think:
i never took issue with object aliases, but may have mentioned them in
passing when i proposed the improved user error messages for system aliases
in GT::Tools::resolve_alias. if my wording somewhere, somehow suggests
that then point me to those words and i'll try to explain them. certainly
my proposed improvement to system alias error messages didn't address
and never intended to address object aliases.
when i reviewed your change ([GT] SVN Commit r607 - trunk/GT) to GT::Tools::resolve_alias
i took exception to the regex change from /\|/ to <all possible gt components>
as not being understood and not solving the fundamental question: "is this system
alias fully expanded?" this was discussed in the improved error
messages for system aliases as i proposed. we agreed that /:/ does resolve
question is the expansion complete?. and that regex change is in this
subject change notice.
however, i still think that with that regex change we very slightly broke method
GT::Tools::resolve_alias by removing the only check there was that checked
for and returned a "system" (e.g. the regex /\|/ did that). how severe this
breakage is will depend on the using script. if there are adequate defaults
that define a minimal system within the script then the damage is slight,
however, as we know these scripts are if anything inconsistent. but that is
still off-topic a bit.
however, the bulk of the change in this notice involves object_aliases.
it is these changes that i think might be handled differently.
the improvements to object_aliases all discussed above are appropriate,
i just don't think the implemented change is the best approach from a
systems point of view.
my objections are
1) a significant operational change to GT::Conf::load
moving the object alias file loading into GT::Conf::load
is the objection. GT::Conf::load never did that in the past,
that is a big change especially considering GT::Conf::load
is was designed from the start to load only the users options
file.
i see the logic in having GT::Conf::load also load the object
alias files but i don't like it much; primarily because it forces
alias file loading when the files exist, even when not needed.
we agree, before the change the files didn't load unless an
object alias was detected ...
2) your concern about "Avoid repeated loading of object aliases."
again, we agree on this point. but i think there is a far simpler
solution more fully shown below that yields the exact same result
(e.g. encounter an object alias, load the object alias files (one time only)).
also note that this alternate might be (very slightly) superior in
that it doesn't load object aliases files unless they are needed.
3) while not part of this change notice i think it bears repeating: removal
of the "@" object aliases marker will change everything for the worse,
as i've explained in other posts.
basically the change i'd suggest for improving object alias file loading
(prior to the changes by this subject change notice) would be something like
the following change to only GT::Tools::resolve_object_alias:
(the + in column 1 denotes added line -- 5 lines are the entire change)
+my $alias_loaded = 0; # semaphore to inhibit multi alias loadings
sub resolve_object_alias {
my ($alias, @param) = (@_);
+ goto LOOKUP unless $alias_loaded == 0; # skip alias loading if already done
+ ++$alias_loaded;
+
# Load the various definition of aliases
GT::Conf::default('Path::Aliases::Signals', '/usr/share/geniustrader/aliases/signals');
GT::Conf::default('Path::Aliases::Indicators', '/usr/share/geniustrader/aliases/indicators');
GT::Conf::default('Path::Aliases::Systems', '/usr/share/geniustrader/aliases/systems');
GT::Conf::default('Path::Aliases::CloseStrategy', '/usr/share/geniustrader/aliases/closestrategy');
GT::Conf::default('Path::Aliases::MoneyManagement', '/usr/share/geniustrader/aliases/moneymanagement');
GT::Conf::default('Path::Aliases::TradeFilters', '/usr/share/geniustrader/aliases/tradefilters');
GT::Conf::default('Path::Aliases::OrderFactory', '/usr/share/geniustrader/aliases/orderfactory');
GT::Conf::default('Path::Aliases::Analyzers', '/usr/share/geniustrader/aliases/analyzers');
foreach my $kind ("Signals", "Indicators", "Systems", "CloseStrategy",
"MoneyManagement", "TradeFilters", "OrderFactory",
"Analyzers")
{
foreach my $file (GT::Conf::_get_home_path()."/.gt/aliases/".lc($kind),
GT::Conf::get("Path::Aliases::$kind"))
{
next if not -e $file;
open(ALIAS, "<", "$file") || die "Can't open $file : $!\n";
while (defined($_=<ALIAS>)) {
if (/^\s*(\S+)\s+(.*)$/) {
GT::Conf::default("Aliases::$kind\::$1", $2);
}
}
close ALIAS;
}
}
+LOOKUP:
# Lookup the alias
my $def = GT::Conf::get("Aliases\::$alias");
.
.
.
i don't have strong feelings for leaving in or removing the additional paths to
/usr/share/geniustrader/aliases. i agree they are an oddity in the myriad of
paths in the gt system, add little for the user, add unnecessary overhead, add
complexity. ok, they should go!
nor do i have strong feelings that the goto loop shouldn't be an if conditional.
i do think that moving the object alias file loading to GT::Conf::load from
GT::Tools::resolve_object_alias isn't the right solution.
there is one other issue with all this and that's the two places
that are potentially searched for object_alias files via the inner foreach loop:
GT::Conf::_get_home_path()."/.gt/aliases/".lc($kind)
and
GT::Conf::get("Path::Aliases::$kind")
i don't have strong feelings that a change is needed, but if the default
alias paths (e.g. /usr/share/geniustrader/aliases/$kind) are removed,
is it still necessary to allow the user to specify Path::Aliases::$kind via
the options file (e.g. GT::Conf::get("Path::Aliases::$kind"))?
aloha
ras