Rajeev S writes:
> I feel that it will make the code cluttered. Since the CLI code
> is independent of the rest of mailman client, won't it be
> better to maintain the CLI code in a separate folder, as it is now?
Of course. I didn't explain myself well. What you have now is
mailman/ -+- client/ -+- _client.py
+- docs/
+- tests/
+- cli/ -+- client.py
+- core/
+- docs/
+- lib/
+- tests/
and what I would like to see is something like
mailman/ -+- client/ -+- _client.py
| +- docs/
| +- tests/
+- cli/ -+- client.py
+- core/
+- docs/
+- lib/
+- tests/
It's only one level higher, but it makes the relationship
(mailman.client provides services to mailman.cli) clearer, and
emphasizes the user app (cli).
> > Is there any code outside of this directory that is yours?
>
> I have made modifications to setup.py, other than that, no.
OK, that's great!
> > It's not clear to me why you define a separate main() function in
> > this script -- it doesn't make sense to import it as a module.
>
> I was not quite familiar with using the python setuptools and I
> assumed that such a format was necessary to specify module entry
> points in setup.py. I have now removed the main function from
> mmclient.py
OK. It's up to you (and maybe Barry has a stylistic opinion on it).
> Yikes! Sorry about that. Fixed at least ten [typos] :)
And I thought I was being a cranky old man. Glad we caught those. :-)
> > In lib/utils.py class Filter, what are the class attributes for? They
> > are not used or useful.
>
> The Filter class is used to filter out objects [...]
>
> All the class attributes are being used in the process described above.
I don't see where. In Filter, you have:
class Filter():
key = None
value = None
operator = None
data_set = []
utils = Utils()
def __init__(self, key, value, operator, data):
self.key = key
self.value = value
self.operator = operator
self.data_set = data
So as soon as you instance a Filter, __init__() creates instance
attributes which shadow the class attributes (except utils). I don't
see any references to Filter.<attr> or to super(), so I don't see how
you can be using the class attributes.
> Now that the connection part is managed by a single function
> (Utils.connect), I will move the connection checking part to that
> function.
OK.
> Those lists were not stored into variables until some revisions back. I got
> a random thought that it is a bad practice , and replaced that code with
> a variable assignment. It was then when my PEP8 checker returned an error
> stating the presence of an unused variable, which I solved by deleting the
> assigned variable!
;-)
> I will look into the possibility of refactoring in those parts, I
> have some plans in mind. Hope that those work!
That would be very cool! As I say, I'm not confident that it's worth
the work, so be careful about wasting your time just because *I* said
it. (It's not a waste of time if you do it because you think it's an
interesting problem.)
> All the mentioned changes (except the tests) would be completed at
> the latest, by 9th Aug 2014, Saturday. (r 70) Planning to spend the
> weekend on writing new unit tests. (r 71)
Sounds good!
_______________________________________________
Mailman-Developers mailing list
Mailman-Developers@python.org
https://mail.python.org/mailman/listinfo/mailman-developers
Mailman FAQ: http://wiki.list.org/x/AgA3
Searchable Archives:
http://www.mail-archive.com/mailman-developers%40python.org/
Unsubscribe:
https://mail.python.org/mailman/options/mailman-developers/archive%40jab.org
Security Policy: http://wiki.list.org/x/QIA9
