Marcus Hüwe (suse-tux@gmx.de) wrote:
Hi Adam,
On 2013-01-16 23:48:01 +0000, Adam Spiers wrote:
<SNIP>
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".
OK, thanks.
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 Thanks again! -- To unsubscribe, e-mail: opensuse-buildservice+unsubscribe@opensuse.org To contact the owner, e-mail: opensuse-buildservice+owner@opensuse.org