Re: [Zorba-coders] [Merge] lp:~ceejatec/zorba/feature-module-installation into lp:zorba

2011-11-04 Thread Chris Hillery
Looks good in general. Only tried compiling so far but didn't install. I would 
suggest that at
 least somebody with Windows also tries it.

I built and tested (and even debugged) it on Windows, and at least my own test 
cases worked fine.
 
 Minor comments:
 - theURIPath (good name?); theXQPath?

It's used for all URIs, not just .xq files, so I think --uri-path / theURIPath 
is more accurate.
 
 - setLibPath or setLibPaths in the public api?

IMHO, a path is a list of directories. See $PATH in shell, $CLASSPATH in 
Java, etc. So I consistently used path for the URI path and Library path. I 
couldn't change setModulePaths(), but actually that's kind of accurate because 
what that function now does is set both the URI and Library paths 
simultaneously.

I spelled out that path == list of directories in comments and the --help 
output, so hopefully it's not confusing.
 
 - should we really remove -module-path from zorbacmd but keep it in the 
 public sctx (backwards
 incompatible?)

I don't think so. I left --module-path there because I assumed people were 
using it. It is sometimes convenient as well. I originally considered marking 
it as deprecated and I'm still not sure that it shouldn't be...
 
 - same for ZORBA_MODULES_INSTALL_DIR

Hm. The variable is only for CMake. I didn't think that backwards 
compatibility extended all the way to build configuration variables. I don't 
think it's possible to maintain that variable even as a convenience wrapper, 
because it would break the core/non-core split.
 
 Will you add documentation  change log before merging?

That is now done.

-- 
https://code.launchpad.net/~ceejatec/zorba/feature-module-installation/+merge/80040
Your team Zorba Coders is subscribed to branch lp:zorba.

-- 
Mailing list: https://launchpad.net/~zorba-coders
Post to : zorba-coders@lists.launchpad.net
Unsubscribe : https://launchpad.net/~zorba-coders
More help   : https://help.launchpad.net/ListHelp


Re: [Zorba-coders] [Merge] lp:~ceejatec/zorba/feature-module-installation into lp:zorba

2011-11-04 Thread Zorba Build Bot
Voting does not meet specified criteria. Required: Approve  1, Disapprove  1. 
Got: 1 Approve, 1 Needs Information.
-- 
https://code.launchpad.net/~ceejatec/zorba/feature-module-installation/+merge/80040
Your team Zorba Coders is subscribed to branch lp:zorba.

-- 
Mailing list: https://launchpad.net/~zorba-coders
Post to : zorba-coders@lists.launchpad.net
Unsubscribe : https://launchpad.net/~zorba-coders
More help   : https://help.launchpad.net/ListHelp


Re: [Zorba-coders] [Merge] lp:~ceejatec/zorba/feature-module-installation into lp:zorba

2011-11-04 Thread Matthias Brantner
Review: Approve


-- 
https://code.launchpad.net/~ceejatec/zorba/feature-module-installation/+merge/80040
Your team Zorba Coders is subscribed to branch lp:zorba.

-- 
Mailing list: https://launchpad.net/~zorba-coders
Post to : zorba-coders@lists.launchpad.net
Unsubscribe : https://launchpad.net/~zorba-coders
More help   : https://help.launchpad.net/ListHelp


Re: [Zorba-coders] [Merge] lp:~ceejatec/zorba/feature-module-installation into lp:zorba

2011-10-21 Thread Chris Hillery
Review: Approve


-- 
https://code.launchpad.net/~ceejatec/zorba/feature-module-installation/+merge/80040
Your team Zorba Coders is subscribed to branch lp:zorba.

-- 
Mailing list: https://launchpad.net/~zorba-coders
Post to : zorba-coders@lists.launchpad.net
Unsubscribe : https://launchpad.net/~zorba-coders
More help   : https://help.launchpad.net/ListHelp


Re: [Zorba-coders] [Merge] lp:~ceejatec/zorba/feature-module-installation into lp:zorba

2011-10-21 Thread Chris Hillery
I realize this needs Changelog and doc updates.
-- 
https://code.launchpad.net/~ceejatec/zorba/feature-module-installation/+merge/80040
Your team Zorba Coders is subscribed to branch lp:zorba.

-- 
Mailing list: https://launchpad.net/~zorba-coders
Post to : zorba-coders@lists.launchpad.net
Unsubscribe : https://launchpad.net/~zorba-coders
More help   : https://help.launchpad.net/ListHelp


Re: [Zorba-coders] [Merge] lp:~ceejatec/zorba/feature-module-installation into lp:zorba

2011-10-21 Thread Zorba Build Bot
Voting does not meet specified criteria. Required: Approve  1, Disapprove  1. 
Got: 1 Approve, 1 Pending.
-- 
https://code.launchpad.net/~ceejatec/zorba/feature-module-installation/+merge/80040
Your team Zorba Coders is subscribed to branch lp:zorba.

-- 
Mailing list: https://launchpad.net/~zorba-coders
Post to : zorba-coders@lists.launchpad.net
Unsubscribe : https://launchpad.net/~zorba-coders
More help   : https://help.launchpad.net/ListHelp


Re: [Zorba-coders] [Merge] lp:~ceejatec/zorba/feature-module-installation into lp:zorba

2011-10-21 Thread Matthias Brantner
Review: Needs Information

Looks good in general. Only tried compiling so far but didn't install. I would 
suggest that at least somebody with Windows also tries it.

Minor comments:
- theURIPath (good name?); theXQPath?
- setLibPath or setLibPaths in the public api?
- should we really remove -module-path from zorbacmd but keep it in the public 
sctx (backwards incompatible?)
  - same for ZORBA_MODULES_INSTALL_DIR


Will you add documentation  change log before merging?
-- 
https://code.launchpad.net/~ceejatec/zorba/feature-module-installation/+merge/80040
Your team Zorba Coders is subscribed to branch lp:zorba.

-- 
Mailing list: https://launchpad.net/~zorba-coders
Post to : zorba-coders@lists.launchpad.net
Unsubscribe : https://launchpad.net/~zorba-coders
More help   : https://help.launchpad.net/ListHelp