Marcus Hüwe (firstname.lastname@example.org) wrote:
On 2013-01-16 23:48:01 +0000, Adam Spiers wrote:
As with the prdiff plugin I announced earlier today, I think there's a good argument for including it in the core rather than packaging it as a plugin, but I'd welcome opinions on that.
Hmm what about adding these additional information to the existing "info" command? I'm not sure if a new command is needed for it. What do you think?
Some comments/thoughts about the code:
- parse_xml: It should raise an OscIOError if the file doesn't exist (instead of OscBaseError (this class is only used as a base class for all osc related exceptions)).
Oops right :) I'll fix that.
Raising a NoWorkingCopy exception in case of a SyntaxError is "wrong" because it _is_ a (valid) wc (just the _link file is corrupt).
Also true - late night hacking is not good ... So maybe I should just drop the try/except so the SyntaxError bubbles up?
- _classify_dir: An _aggregate might aggregate packages from multiple projects (that is there might be multiple <aggregate /> elements).
Ah, I didn't know that.
Currently your code only considers the last <aggregate /> element.
Yup, fixing ...
If you want to check if a '_link' file is present in a package you can simply use "if '_link' in pkg.filenamelist..." instead of directly inspecting the storedir. In order to retrieve the contents of a storefile you should use "store_read_file".
- do_classify: Use "is_project_dir" and "is_package_dir".
As I already wrote in my other mail feel free to ignore the comments from above - most of them are just "cosmetic":)
Nope, they are very good comments and the fixes are now in my github repo :)
I'll defer working on merging this into osc until:
- we've resolved the Travis issues - I've done a pull request for prdiff into osc