Code review comment for lp:~marco-gallotta/ibid/ascii-501638

Revision history for this message
marcog (marco-gallotta) wrote :

All these issues should be resolved in the latest commits.

I did a bit of refactoring on the resizing:
* input syntax changes from [<width>x<height>] to [width <width>] [height <height>], so that just one dimension can be set and the other determined from aspect ratio.
* I've made the assumed font size (used to work out the dimensions in text form) configurable; I think the defaults are quite reasonable but feel free to suggest alternatives.
* lowered the default height and selected what I feel are sane defaults for max_{width,height}.
* if the aspect ratio would make the calculated dimension too large, it squashes the image rather than throwing an error.
* if the supplied dimensions are too large, throw an error.

> > === modified file 'ibid.ini'
>
> Somehow you modified ibid.ini. (probably when merging from trunk).
>
> > (?:\s+(in colou?r))?
>
> You only compare this string to none, so you can say r'(\s+in\s+colou?r)?'
>
> > event.addresponse(u'File too large (limit is', self.max_size, 'KiB)')
>
> No, not quite like that. You still do a substitution, but it does it for you:
>
> event.addresponse(u'File too large (limit is %i KiB)', self.max_size)
>
> > event.addresponse(response.replace('\r', ''), address=False, conflate=False)
>
> I suspect that'll throw a unicode warning.
>
> > fonts_zip = Option('fonts_zip', 'Zip file containing figlet fonts',
> '/tmp/pyfiglet-0.4/fonts.zip')
>
> We can't merge with that default. I'd make the path relative to
> ibid.config.base (the botdir).
> I.e. 'data/figlet-fonts.zip'
>
> Overall, I'd still like some limits. Maximum width & height of all possible
> ascii art output.
> If it sees that it's making something too big, it should just return something
> like
> "I'm sorry, I can't draw ascii graffiti that big"

review: Needs Resubmitting

« Back to merge proposal