I noticed Cobbler does not apply some conventions used in other Python projects. I have some suggestions:
* Validate code using Pylint * Comment: I think this is fundamental to find subtle errors (eg referencing inexistent variables). I have already fixed all Pylint errors I could in my Cobbler clone and I just ignored those few I cannot fix (eg in templating classes). I could submit relevant patches upstream. I'll soon automate Pylint checks in any patch submitted by my team, possibly Cobbler upstream could do the same.
* Introduce documentation convention * Motivation: makes documentation easier to read and write * Suggestions: * Sphinx restructuredText extensions (http://sphinx-doc.org/domains.html#signatures) . * simple Javadoc-style documentation convention. eg document methods are documented with method description, @param <type> <var_name> <var_description> for each parameter and @return <type> <return_var_description> for return value. My team uses that and it works well. * Comment: I consider param/return type specially useful, as Python does not declare variable types in method definition.
* Validate code using PEP8 (Python style guide)
* Order imports alphabetically. Possibly, split imports in standard Python imports, third-party imports and Cobbler imports. * Motivation: readability
* Remove trailing whitespaces * Motivation: remove commit noise. I see other reasons at http://programmers.stackexchange.com/questions/121555/why-is-trailing-whites... , but imho they are minor.
Maybe some of these suggestions have already been considered in the past and rejected. If you could share with me these reasons, it'd be much appreciated.
Regards, Alan Evangelista
Hi Alan,
Well, again. I could't agree more with you. I have marked this conversation as something that should be done during the Cobbler 2.7 development cycle. I will follow up on this after the 2.6 release, which i finally got around to right now.
Feel free to send any patches targeting master!
Once again, thanks!
On Mon, Mar 24, 2014 at 4:37 PM, Alan Evangelista <alanoe@linux.vnet.ibm.com
wrote:
I noticed Cobbler does not apply some conventions used in other Python projects. I have some suggestions:
- Validate code using Pylint
- Comment: I think this is fundamental to find subtle errors (eg
referencing inexistent variables). I have already fixed all Pylint errors I could in my Cobbler clone and I just ignored those few I cannot fix (eg in templating classes). I could submit relevant patches upstream. I'll soon automate Pylint checks in any patch submitted by my team, possibly Cobbler upstream could do the same.
- Introduce documentation convention
- Motivation: makes documentation easier to read and write
- Suggestions:
- Sphinx restructuredText extensions (http://sphinx-doc.org/
domains.html#signatures) .
- simple Javadoc-style documentation convention. eg document methods are
documented with method description, @param <type> <var_name> <var_description> for each parameter and @return <type> <return_var_description> for return value. My team uses that and it works well.
- Comment: I consider param/return type specially useful, as Python does
not declare variable types in method definition.
Validate code using PEP8 (Python style guide)
Order imports alphabetically. Possibly, split imports in standard Python
imports, third-party imports and Cobbler imports.
Motivation: readability
Remove trailing whitespaces
Motivation: remove commit noise. I see other reasons at
http://programmers.stackexchange.com/questions/121555/why-is-trailing- whitespace-a-big-deal , but imho they are minor.
Maybe some of these suggestions have already been considered in the past and rejected. If you could share with me these reasons, it'd be much appreciated.
Regards, Alan Evangelista
cobbler-devel mailing list cobbler-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/cobbler-devel
Hi Alan, others,
With 2.6.0 out of the way, i found some time to play with pyflakes and pep8.I just finished fixing a gazillion issues throughout the codebase like unused imports, unreferenced variables, and stuff like that. That also resulted in the removal of some legacy code (< python 2.5 hacks) and i also fixed a bug or two. Pyflakes is now integrated into the Makefile (make qa) and is run when building tarballs and rpms so that any future issues are caught early on.
The next step is integrating pep8 validation, which from the looks of it will be a hell of a job :-)
Looking forward to your patches (please make sure you rebase from current master) and feedback.
On Wed, Apr 9, 2014 at 3:23 PM, Jörgen Maas jorgen.maas@gmail.com wrote:
Hi Alan,
Well, again. I could't agree more with you. I have marked this conversation as something that should be done during the Cobbler 2.7 development cycle. I will follow up on this after the 2.6 release, which i finally got around to right now.
Feel free to send any patches targeting master!
Once again, thanks!
On Mon, Mar 24, 2014 at 4:37 PM, Alan Evangelista < alanoe@linux.vnet.ibm.com> wrote:
I noticed Cobbler does not apply some conventions used in other Python projects. I have some suggestions:
- Validate code using Pylint
- Comment: I think this is fundamental to find subtle errors (eg
referencing inexistent variables). I have already fixed all Pylint errors I could in my Cobbler clone and I just ignored those few I cannot fix (eg in templating classes). I could submit relevant patches upstream. I'll soon automate Pylint checks in any patch submitted by my team, possibly Cobbler upstream could do the same.
- Introduce documentation convention
- Motivation: makes documentation easier to read and write
- Suggestions:
- Sphinx restructuredText extensions (http://sphinx-doc.org/
domains.html#signatures) .
- simple Javadoc-style documentation convention. eg document methods
are documented with method description, @param <type> <var_name> <var_description> for each parameter and @return <type> <return_var_description> for return value. My team uses that and it works well.
- Comment: I consider param/return type specially useful, as Python does
not declare variable types in method definition.
Validate code using PEP8 (Python style guide)
Order imports alphabetically. Possibly, split imports in standard
Python imports, third-party imports and Cobbler imports.
Motivation: readability
Remove trailing whitespaces
Motivation: remove commit noise. I see other reasons at
http://programmers.stackexchange.com/questions/121555/why-is-trailing- whitespace-a-big-deal , but imho they are minor.
Maybe some of these suggestions have already been considered in the past and rejected. If you could share with me these reasons, it'd be much appreciated.
Regards, Alan Evangelista
cobbler-devel mailing list cobbler-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/cobbler-devel
-- Grtz, Jörgen Maas
Hey all,
Just finished integrating the pyflakes and pep8 validation. We now have that in place for cobbler and cobbler-web, koan will be addressed a bit later. Just wanted to let you know that ALL files have changed so if you have any work pending a pull request, please rebase first against current master.
Also, if you want to contribute to Cobbler please make sure your patches pass pyflakes and pep8 by using 'make qa'. Next up will be updating the Developer Guide so that all of this is properly documented.
Cheers!
On Wed, Apr 16, 2014 at 7:44 PM, Jörgen Maas jorgen.maas@gmail.com wrote:
Hi Alan, others,
With 2.6.0 out of the way, i found some time to play with pyflakes and pep8.I just finished fixing a gazillion issues throughout the codebase like unused imports, unreferenced variables, and stuff like that. That also resulted in the removal of some legacy code (< python 2.5 hacks) and i also fixed a bug or two. Pyflakes is now integrated into the Makefile (make qa) and is run when building tarballs and rpms so that any future issues are caught early on.
The next step is integrating pep8 validation, which from the looks of it will be a hell of a job :-)
Looking forward to your patches (please make sure you rebase from current master) and feedback.
On Wed, Apr 9, 2014 at 3:23 PM, Jörgen Maas jorgen.maas@gmail.com wrote:
Hi Alan,
Well, again. I could't agree more with you. I have marked this conversation as something that should be done during the Cobbler 2.7 development cycle. I will follow up on this after the 2.6 release, which i finally got around to right now.
Feel free to send any patches targeting master!
Once again, thanks!
On Mon, Mar 24, 2014 at 4:37 PM, Alan Evangelista < alanoe@linux.vnet.ibm.com> wrote:
I noticed Cobbler does not apply some conventions used in other Python projects. I have some suggestions:
- Validate code using Pylint
- Comment: I think this is fundamental to find subtle errors (eg
referencing inexistent variables). I have already fixed all Pylint errors I could in my Cobbler clone and I just ignored those few I cannot fix (eg in templating classes). I could submit relevant patches upstream. I'll soon automate Pylint checks in any patch submitted by my team, possibly Cobbler upstream could do the same.
- Introduce documentation convention
- Motivation: makes documentation easier to read and write
- Suggestions:
- Sphinx restructuredText extensions (http://sphinx-doc.org/
domains.html#signatures) .
- simple Javadoc-style documentation convention. eg document methods
are documented with method description, @param <type> <var_name> <var_description> for each parameter and @return <type> <return_var_description> for return value. My team uses that and it works well.
- Comment: I consider param/return type specially useful, as Python
does not declare variable types in method definition.
Validate code using PEP8 (Python style guide)
Order imports alphabetically. Possibly, split imports in standard
Python imports, third-party imports and Cobbler imports.
Motivation: readability
Remove trailing whitespaces
Motivation: remove commit noise. I see other reasons at
http://programmers.stackexchange.com/questions/121555/why-is-trailing- whitespace-a-big-deal , but imho they are minor.
Maybe some of these suggestions have already been considered in the past and rejected. If you could share with me these reasons, it'd be much appreciated.
Regards, Alan Evangelista
cobbler-devel mailing list cobbler-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/cobbler-devel
-- Grtz, Jörgen Maas
-- Grtz, Jörgen Maas
On 04/27/2014 06:17 AM, Jörgen Maas wrote:
Hey all,
Just finished integrating the pyflakes and pep8 validation. We now have that in place for cobbler and cobbler-web, koan will be addressed a bit later. Just wanted to let you know that ALL files have changed so if you have any work pending a pull request, please rebase first against current master.
Also, if you want to contribute to Cobbler please make sure your patches pass pyflakes and pep8 by using 'make qa'. Next up will be updating the Developer Guide so that all of this is properly documented.
Cheers!
Sorry for taking so long to post a feedback. Thanks for integrating pyflakes/pep8 integration and making several fixes, Jörgen.
1) Strangely, I downloaded Cobbler's Git repository master branch, run 'make qa' and got the following output:
$ make qa running pyflakes... cobbler/modules/serializer_mysql.py:23:3: invalid syntax """ ^ make: *** [qa] Error 1
I was supposing master branch would be approved by Pyflakes. I am not familiar with Pyflakes, so maybe I am missing something?
Also, I see errors when I run pep8 for all Python files in Cobbler source code (even ignoring errors E303 and E501). I was also supposing master branch would be approved by PEP8.
2) Curiosity: is there a reason why you chose Pyflakes over Pylint? Maybe because it is less strict and less verbose?
3) I rebased against master and just made a pull request with several Pylint fixes. I'll wait your feedback.
Regards, Alan Evangelista
On Tue, May 20, 2014 at 7:31 PM, Alan Evangelista <alanoe@linux.vnet.ibm.com
wrote:
On 04/27/2014 06:17 AM, Jörgen Maas wrote:
Hey all,
Just finished integrating the pyflakes and pep8 validation. We now have that in place for cobbler and cobbler-web, koan will be addressed a bit later. Just wanted to let you know that ALL files have changed so if you have any work pending a pull request, please rebase first against current master.
Also, if you want to contribute to Cobbler please make sure your patches pass pyflakes and pep8 by using 'make qa'. Next up will be updating the Developer Guide so that all of this is properly documented.
Cheers!
Sorry for taking so long to post a feedback. Thanks for integrating pyflakes/pep8 integration and making several fixes, Jörgen.
- Strangely, I downloaded Cobbler's Git repository master branch, run
'make qa' and got the following output:
$ make qa running pyflakes... cobbler/modules/serializer_mysql.py:23:3: invalid syntax """ ^ make: *** [qa] Error 1
I was supposing master branch would be approved by Pyflakes. I am not familiar with Pyflakes, so maybe I am missing something?
Also, I see errors when I run pep8 for all Python files in Cobbler source code (even ignoring errors E303 and E501). I was also supposing master branch would be approved by PEP8.
- Curiosity: is there a reason why you chose Pyflakes over Pylint? Maybe
because it is less strict and less verbose?
- I rebased against master and just made a pull request with several
Pylint fixes. I'll wait your feedback.
Regards, Alan Evangelista
cobbler-devel mailing list cobbler-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/cobbler-devel
Hi Alan,
Strange indeed, i'm running my dev env on centos6 (thus targeting python 2.6). Perhaps your setup is more current?
Indeed i found pylint somewhat too much (work) for now, but i do see value in integrating pylint as well. Just have to make some time to read more about it, so if you have any patches laying around for doing just that i'm happy to merge.
I will take a look at all pull requests tonight, thanks again!
cobbler-devel@lists.fedorahosted.org