Merge lp://staging/~slacklinucs/lalita/new-photo into lp://staging/lalita

Hey, great functionality; some details are pending, let's work to push this:

Notes:

- The "photo_command" method needs to keep the docstring, because it's the command's help.

- You mixed tabs with spaces! Need to indent using only spaces.

- Don't add this plugin to lalita.cfg.sample, the idea is to keep it simple

- Tests look ok, but couldn't run it because you mixed tabs with spaces.

So, let me know when you push these fixes!

Thank you!

Facundo,

Requested fixes were applied, please consider merging revision 171 into
trunk.

Thanks!

Martín

On Mon, Apr 4, 2011 at 8:18 PM, Facundo Batista <email address hidden>wrote:

> Review: Needs Fixing
> Hey, great functionality; some details are pending, let's work to push
> this:
>
> Notes:
>
> - The "photo_command" method needs to keep the docstring, because it's the
> command's help.
>
> - You mixed tabs with spaces! Need to indent using only spaces.
>
> - Don't add this plugin to lalita.cfg.sample, the idea is to keep it simple
>
> - Tests look ok, but couldn't run it because you mixed tabs with spaces.
>
> So, let me know when you push these fixes!
>
> Thank you!
> --
> https://code.launchpad.net/~slacklinucs/lalita/new-photo/+merge/56164
> You are the owner of lp:~slacklinucs/lalita/new-photo.
>

Some further comments:

- Docstrings in command and private message are ok... but is redundant in _photo(), please put a doscstring there like "Common photo functionality", or something.

- In photo_command(), why you do:

        if retTxt:
            self.say(channel, retTxt)

...if retTxt is always true? (same in photo_private())

- Please merge again lalita.cfg.sample, it seems to be a whitespace conflict.

Thank you!

Hi!

I still have a conflict in the config file.

However, this file is not need, so I used the other two, and merged them to trunk.

Thanks!