My main concern here is that this refactoring doesn't take into account properties
that only need get or get/set accessors instead of full access. For the contributor that
isn't thinking about it, this may open up unintentional use of the properties down the
line. Quite honestly, my early use of properties in image-factory could stand some
cleanup in terms of read, read/write, read/write/delete and I worry that hiding that will
only exacerbate the issue.
Mark and I chatted about refining the prop code to allow for specifying the accessors you
want a property to provide. We agreed to move forward with the current prop code and add
a task to add the accessor specifiers.
so, ACK
-steve
On Jun 3, 2011, at 10:25 AM, Jim Meyering wrote:
Steve Loranz wrote:
> NACK
>
> This is personal preference. We could argue the merits of both styles
> like we could argue Emacs vs Vi.
>
> On Jun 1, 2011, at 6:22 AM, Mark McLoughlin wrote:
>
>> We have a lot of copy and paste code in our property definitions.
>>
>> Split this code out into utility methods and re-use it.
>>
>> (As the cool kids would say, "DRY up the property definitions").
>> ---
>> imagefactory/ApplicationConfiguration.py | 12 +---
>> imagefactory/ImageWarehouse.py | 63 ++---------------
>> imagefactory/Template.py | 37 +---------
>> imagefactory/builders/BaseBuilder.py | 109 +++-------------------------
>> imagefactory/props.py | 47 ++++++++++++
>> imagefactory/qmfagent/BuildAdaptor.py | 74 ++-----------------
>> imagefactory/qmfagent/ImageFactory.py | 25 +------
>> imagefactory/qmfagent/ImageFactoryAgent.py | 19 +----
>> 8 files changed, 82 insertions(+), 304 deletions(-)
>> create mode 100644 imagefactory/props.py
Hi Steve,
Please reconsider,
304 deletions, and only 82 insertions
That's a nice bit of clean-up.
Do you really want to reject a patch that cleanly factors
out so much duplicate or boiler-plate code?
It's not just a question of style or personal preference.
It's more a preference for minimizing maintenance cost
and for retaining the mental health of our kind reviewers.
Removing duplication means there's less code to review and
if anything needs to be changed, there are far fewer places
to change it.
Jim