Merge lp://qastaging/~piastucki/bzr-xmloutput/xml-shelve-list into lp://qastaging/bzr-xmloutput

Proposed by Piotr Piastucki
Status: Merged
Approved by: Guillermo Gonzalez
Approved revision: 167
Merged at revision: 169
Proposed branch: lp://qastaging/~piastucki/bzr-xmloutput/xml-shelve-list
Merge into: lp://qastaging/bzr-xmloutput
Diff against target: 209 lines (+170/-2)
5 files modified
__init__.py (+2/-1)
cmds.py (+22/-0)
info.py (+1/-1)
shelvexml.py (+44/-0)
tests/test_shelve_xml.py (+101/-0)
To merge this branch: bzr merge lp://qastaging/~piastucki/bzr-xmloutput/xml-shelve-list
Reviewer Review Type Date Requested Status
Guillermo Gonzalez Approve
Review via email: mp+161167@code.qastaging.launchpad.net

This proposal supersedes a proposal from 2013-02-27.

Description of the change

The changes in this branch add xmlshelvelist command, an XML version of "bzr shelve --list".

To post a comment you must log in.
Revision history for this message
Guillermo Gonzalez (verterok) wrote : Posted in a previous version of this proposal

The code looks good.
Just one question, any reason to add the specific xmlshelvelist command instead of a command + option like the shelve command, e.g: xmlshelve --list?

Thanks

review: Needs Information
Revision history for this message
Piotr Piastucki (piastucki) wrote : Posted in a previous version of this proposal

Good point. I was also thinking about that. I decided to add a new command as the main purpose of the shelve command is shelving files and --list option seems to be secondary. I think it is a good idea to provide XML versions of the commands that generate complex output and avoid re-implementing commands that modify anything. In this case I could add xmlshelve command that would support only --list option and throw an error otherwise. (or maybe delegate to built-in commands instead?). Instead of adding a very limited xmlshelve command I added xmlshelvelist to make it more obvious what the command actually does. However, I am not fully convinced this is the best apporach either. Please let me know what you think and I will change the code accordingly.

Revision history for this message
Piotr Piastucki (piastucki) wrote : Posted in a previous version of this proposal

> The code looks good.
> Just one question, any reason to add the specific xmlshelvelist command
> instead of a command + option like the shelve command, e.g: xmlshelve --list?
>
> Thanks

Good point. I was also thinking about that. I decided to add a new command as the main purpose of the shelve command is shelving files and --list option seems to be secondary. I think it is a good idea to provide XML versions of the commands that generate complex output and avoid re-implementing commands that modify anything. In this case I could add xmlshelve command that would support only --list option and throw an error otherwise. (or maybe delegate to built-in commands instead?). Instead of adding a very limited xmlshelve command I added xmlshelvelist to make it more obvious what the command actually does. However, I am not fully convinced this is the best apporach either. Please let me know what you think and I will change the code accordingly.

Revision history for this message
Guillermo Gonzalez (verterok) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to all changes: