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 = dataSo 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!