Code review comment for lp:~weyrick/bzr/54624-warn-on-large-files

Revision history for this message
Martin Pool (mbp) wrote :

On 9 August 2011 01:01, Shannon Weyrick <email address hidden> wrote:
>> Thanks for this Shannon, I'm sure it will save some users some grief.
>>
>> I think the default 'large' should probably be set at the kind of size
>> where there's a fair chance people would regret adding it, because of
>> the impact on disk/memory usage/transfer time.  1MB shouldn't be a
>> problem for any of them and I think it's fairly likely people will
>> have assets that big (if not actual source files.)  Computers are
>> bigger than they were when Vesta was new...
>>
>> I think say 50MB would be nearer the mark.
>>
>> I don't think it's especially worth scanning Launchpad branches unless
>> you really want to.  You might just as well look in your own home
>> directory or perhaps ask on the list.
>>
>
> I agree 1 MB is almost definitely too low, otoh 50 sounds high to me. I was thinking something on the order of 10-20MB - that almost definitely covers all source files and most normal resources from web projects as well. But, I'm not stuck on that number.

10-20MB is also fine with me.

> What would be real handy here is the first time this limit was reached, it could prompt the user if they wanted to raise/set it. Is there infrastructure in place in the ui for that kind of thing?

There is some infrastructure towards that - actually this reminds me
of something I should have said before, which is that rather than
trace.warning you should use ui_factory.warning - that takes a
structure identifier of the type of warning, with the idea that
eventually we can give the user an option to not show the warning any
more, or perhaps to change it.

> Actually, I think what we really want is to not force the user to enter an integer for this, but rather "20M" or "1G". I could whip something up for this and stick it in Config.

That would be great.

> Is there a central place defaults/option names are defined? I didn't see one.

There is option_registry in config.py.

« Back to merge proposal