Code review comment for lp:~savoirfairelinux-openerp/lp-community-utils/nag_refactor
Revision history for this message
| Guewen Baconnier @ Camptocamp (gbaconnier-c2c) wrote | # |
review:
Needs Fixing
(code review)
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
73416ba
(Get the code!)
Sorry I didn't saw your proposal before (even if I use openerp-nag...) whereas it deserves some attention.
Thanks for your work so far.
Seems good to me even if some changes seems not really necessary to me, surely you had some reasons.
What is the rationale to change the Nag namedtuple to a class? A reason could be that you added methods, but not. On that topic, couldn't we move the big print of the end to a method on Nag() (it seems that it could even be the __str__).
Couldn't already the module be used as a lib? (as there is if __name__ == "__main__":) If the filename is an issue (it is, i we want to import it), maybe we can consider to rename it? Not that I strongly disagree with the split but it seem a bit overkill to me here. But maybe do you have great plans to extend this tool and so it makes more sense ;-)
I strongly disagree to remove the --authenticated option: the Launchpad API is limited with anonymous users and it is used now for the votes (the votes are only visible by authenticated users). BTW you may want to rebase on the master since votes have been integrated meanwhile.
What was the bug with the age?