Code review comment for lp:~mvo/click/fix-multiple-framework-validation

Revision history for this message
Michael Vogt (mvo) wrote :

Hi Barry,

thanks for your review!

On Tue, Feb 17, 2015 at 04:03:34PM -0000, Barry Warsaw wrote:
[..]
> Would it make sense to wrap this in `try/except ImportError` clauses? I know that gets a bit verbose, but at least you wouldn't have these commented out lines and could automatically use the native libs if available.

Thanks, I was thinking about this a little bit and I think this block
can just be removed. Its a goal of click build to run on pure
python. So we need the function anyway and can as well use them :)

[..]
> > + framework_base_name = click_framework_get_base_name(
> > + framework_name)
> > framework_base_version = click_framework_get_base_version(
> > framework_name)
> > - framework_base_versions.add(framework_base_version)
> > + if (framework_base_name in base_name_versions and
> > + framework_base_version != base_name_versions[framework_base_name]):
>
> What about (w/abbreviations due to wrapping):
>
> prev = base_name_versions.get(framework_base_name, framework_base_version)
> if framework_base_version != prev:
>
> I guess it may not save that much.

I like this idea! Commited.

Feel free to set to "approved" if you feel its ready.

Thanks,
 Michael

« Back to merge proposal