Hi,
Since there is no coverage report on tests in vdsm, if a PEP 8 patch passes the tests, we still not sure if there is no mistake in it. Viewing the diff reports on all the changes consumes a lot of time, and some small but fatal mistakes(like misspelling variable name) can easily be ignored by human eyes.
So I have a try on the compiler module of Python. I write a tool named 'pydiff'. pydiff parses two Python scripts into Abstract Syntax Trees. These data structures can reflect the logic of the code, and pydiff performs a recursive compare on the trees. Then pydiff reports differences and the corresponding line numbers. In this way, pydiff ignores code style changes, and reports only logical changes of the code.
I think this tool can save us a lot of time. After a PEP 8 patch passes vdsm tests and pydiff, I will get some confidence on the patch and it probably does not break anything in vdsm.
Here is a usage example:
-------- test_o.py -------- def foo(a, b): pass
if __name__ == '__main__': A = [1, 2, 3] print (4, 5, 6), \ "over" foo(1, 2) print 'Hello World'
-------- test_n.py -------- def foo(a, b): pass
if __name__ == '__main__': A = [1, 2, 3] print (4, 5, 6), "over" fooo( 1, 2) print ('Hello ' 'World')
Some differences of the files are just a matter of style. The only significant difference is the function call "foo()" is misspelled in "test_n.py".
Run pydiff.py, it will report:
$ python pydiff.py test_*.py 1 difference(s) first file: test_n.py second file: test_o.py
((8, 'fooo'), (8, 'foo'))
This report tells us that 'fooo' in line 8 of "test_n.py" is different from 'foo' in line 8 of "test_o.py".
It can also find insertions or deletions. Here is another simple example:
---- old.py ---- print 'Hello 1' print 'Hello 2' print 'Hello 3' print 'Hello 4' print 'Hello 5'
---- new.py ---- print 'Hello 1' print 'Hello 3' print 'Hello 4' print 'Hello 5' print 'Hello 5'
Run pydiff:
$ pydiff old.py new.py 2 difference(s) first file: old.py second file: new.py
((2, Printnl([Const('Hello 2')], None)), (2, None))
((5, None), (5, Printnl([Const('Hello 5')], None)))
Here "((2, Printnl([Const('Hello 2')], None)), (2, None))" means there is a print statement in line 2 of old.py, but no corresponding statement in new.py, so we can know the statement is deleted in new.py. "((5, None), (5, Printnl([Const('Hello 5')], None)))" means there is a print statement in line 5 of new.py, but no corresponding statement in old.py, so we can know the statement is inserted in new.py.
Sometimes the change in code logic is acceptable, for example, change "aDict.has_key(Key)" into "Key in aDict". pydiff can report a difference in this case, but it is up to the user to judge whether it's acceptable. pydiff is just a tool to help you finding these changes.
I hope it can be helpful for PEP 8 patch reviewers. If you find any bugs, please let me know. The script is in the attachment.
I haven't used the tool yet, but saw your mail with the examples. I think its a very nice tool and very helpful. Why don't you submit this tool to python project itself ? It think it deserves it.
On Thu, Jun 07, 2012 at 12:03:30PM +0800, Zhou Zheng Sheng wrote:
Hi,
Since there is no coverage report on tests in vdsm, if a PEP 8 patch passes the tests, we still not sure if there is no mistake in it. Viewing the diff reports on all the changes consumes a lot of time, and some small but fatal mistakes(like misspelling variable name) can easily be ignored by human eyes.
So I have a try on the compiler module of Python. I write a tool named 'pydiff'. pydiff parses two Python scripts into Abstract Syntax Trees. These data structures can reflect the logic of the code, and pydiff performs a recursive compare on the trees. Then pydiff reports differences and the corresponding line numbers. In this way, pydiff ignores code style changes, and reports only logical changes of the code.
I think this tool can save us a lot of time. After a PEP 8 patch passes vdsm tests and pydiff, I will get some confidence on the patch and it probably does not break anything in vdsm.
This is a very nice tool. Thanks for sharing it. I would like to see all authors of PEP8 patches use this to check their patches for semantic correctness. This should greatly improve our ability to complete the PEP8 cleanup quickly.
Here is a usage example:
-------- test_o.py -------- def foo(a, b): pass
if __name__ == '__main__': A = [1, 2, 3] print (4, 5, 6), \ "over" foo(1, 2) print 'Hello World'
-------- test_n.py -------- def foo(a, b): pass
if __name__ == '__main__': A = [1, 2, 3] print (4, 5, 6), "over" fooo( 1, 2) print ('Hello ' 'World')
Some differences of the files are just a matter of style. The only significant difference is the function call "foo()" is misspelled in "test_n.py".
Run pydiff.py, it will report:
$ python pydiff.py test_*.py 1 difference(s) first file: test_n.py second file: test_o.py
((8, 'fooo'), (8, 'foo'))
This report tells us that 'fooo' in line 8 of "test_n.py" is different from 'foo' in line 8 of "test_o.py".
It can also find insertions or deletions. Here is another simple example:
---- old.py ---- print 'Hello 1' print 'Hello 2' print 'Hello 3' print 'Hello 4' print 'Hello 5'
---- new.py ---- print 'Hello 1' print 'Hello 3' print 'Hello 4' print 'Hello 5' print 'Hello 5'
Run pydiff:
$ pydiff old.py new.py 2 difference(s) first file: old.py second file: new.py
((2, Printnl([Const('Hello 2')], None)), (2, None))
((5, None), (5, Printnl([Const('Hello 5')], None)))
Here "((2, Printnl([Const('Hello 2')], None)), (2, None))" means there is a print statement in line 2 of old.py, but no corresponding statement in new.py, so we can know the statement is deleted in new.py. "((5, None), (5, Printnl([Const('Hello 5')], None)))" means there is a print statement in line 5 of new.py, but no corresponding statement in old.py, so we can know the statement is inserted in new.py.
Sometimes the change in code logic is acceptable, for example, change "aDict.has_key(Key)" into "Key in aDict". pydiff can report a difference in this case, but it is up to the user to judge whether it's acceptable. pydiff is just a tool to help you finding these changes.
I hope it can be helpful for PEP 8 patch reviewers. If you find any bugs, please let me know. The script is in the attachment.
-- Thanks and best regards!
Zhou Zheng Sheng / 周征晟 E-mail: zhshzhou@linux.vnet.ibm.com Telephone: 86-10-82454397
vdsm-devel mailing list vdsm-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/vdsm-devel
On 2012-6-7 21:26, Adam Litke wrote:
On Thu, Jun 07, 2012 at 12:03:30PM +0800, Zhou Zheng Sheng wrote:
Hi,
Since there is no coverage report on tests in vdsm, if a PEP 8 patch passes the tests, we still not sure if there is no mistake in it. Viewing the diff reports on all the changes consumes a lot of time, and some small but fatal mistakes(like misspelling variable name) can easily be ignored by human eyes.
So I have a try on the compiler module of Python. I write a tool named 'pydiff'. pydiff parses two Python scripts into Abstract Syntax Trees. These data structures can reflect the logic of the code, and pydiff performs a recursive compare on the trees. Then pydiff reports differences and the corresponding line numbers. In this way, pydiff ignores code style changes, and reports only logical changes of the code.
I think this tool can save us a lot of time. After a PEP 8 patch passes vdsm tests and pydiff, I will get some confidence on the patch and it probably does not break anything in vdsm.
This is a very nice tool. Thanks for sharing it. I would like to see all authors of PEP8 patches use this to check their patches for semantic correctness. This should greatly improve our ability to complete the PEP8 cleanup quickly.
Yes, I agree with you. Also, we should merge this tool into vdsm as a helper for PEP8 clean work.
Here is a usage example:
-------- test_o.py -------- def foo(a, b): pass
if __name__ == '__main__': A = [1, 2, 3] print (4, 5, 6), \ "over" foo(1, 2) print 'Hello World'
-------- test_n.py -------- def foo(a, b): pass
if __name__ == '__main__': A = [1, 2, 3] print (4, 5, 6), "over" fooo( 1, 2) print ('Hello ' 'World')
Some differences of the files are just a matter of style. The only significant difference is the function call "foo()" is misspelled in "test_n.py".
Run pydiff.py, it will report:
$ python pydiff.py test_*.py 1 difference(s) first file: test_n.py second file: test_o.py
((8, 'fooo'), (8, 'foo'))
This report tells us that 'fooo' in line 8 of "test_n.py" is different from 'foo' in line 8 of "test_o.py".
It can also find insertions or deletions. Here is another simple example:
---- old.py ---- print 'Hello 1' print 'Hello 2' print 'Hello 3' print 'Hello 4' print 'Hello 5'
---- new.py ---- print 'Hello 1' print 'Hello 3' print 'Hello 4' print 'Hello 5' print 'Hello 5'
Run pydiff:
$ pydiff old.py new.py 2 difference(s) first file: old.py second file: new.py
((2, Printnl([Const('Hello 2')], None)), (2, None))
((5, None), (5, Printnl([Const('Hello 5')], None)))
Here "((2, Printnl([Const('Hello 2')], None)), (2, None))" means there is a print statement in line 2 of old.py, but no corresponding statement in new.py, so we can know the statement is deleted in new.py. "((5, None), (5, Printnl([Const('Hello 5')], None)))" means there is a print statement in line 5 of new.py, but no corresponding statement in old.py, so we can know the statement is inserted in new.py.
Sometimes the change in code logic is acceptable, for example, change "aDict.has_key(Key)" into "Key in aDict". pydiff can report a difference in this case, but it is up to the user to judge whether it's acceptable. pydiff is just a tool to help you finding these changes.
I hope it can be helpful for PEP 8 patch reviewers. If you find any bugs, please let me know. The script is in the attachment.
-- Thanks and best regards!
Zhou Zheng Sheng / 周征晟 E-mail: zhshzhou@linux.vnet.ibm.com Telephone: 86-10-82454397
vdsm-devel mailing list vdsm-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/vdsm-devel
On Thu, Jun 07, 2012 at 11:13:14PM +0800, Shu Ming wrote:
On 2012-6-7 21:26, Adam Litke wrote:
On Thu, Jun 07, 2012 at 12:03:30PM +0800, Zhou Zheng Sheng wrote:
Hi,
Since there is no coverage report on tests in vdsm, if a PEP 8 patch passes the tests, we still not sure if there is no mistake in it. Viewing the diff reports on all the changes consumes a lot of time, and some small but fatal mistakes(like misspelling variable name) can easily be ignored by human eyes.
So I have a try on the compiler module of Python. I write a tool named 'pydiff'. pydiff parses two Python scripts into Abstract Syntax Trees. These data structures can reflect the logic of the code, and pydiff performs a recursive compare on the trees. Then pydiff reports differences and the corresponding line numbers. In this way, pydiff ignores code style changes, and reports only logical changes of the code.
I think this tool can save us a lot of time. After a PEP 8 patch passes vdsm tests and pydiff, I will get some confidence on the patch and it probably does not break anything in vdsm.
This is a very nice tool. Thanks for sharing it. I would like to see all authors of PEP8 patches use this to check their patches for semantic correctness. This should greatly improve our ability to complete the PEP8 cleanup quickly.
Yes, I agree with you. Also, we should merge this tool into vdsm as a helper for PEP8 clean work.
Thanks, Zhou Zheng! I hope this expedites the pep8 conversion process.
As the tool is not vdsm-specific, I'd rather see it in pypi.python.org than in vdsm.
Regrads, Dan.
On Sun, Jun 10, 2012 at 11:15:48AM +0300, Dan Kenigsberg wrote:
On Thu, Jun 07, 2012 at 11:13:14PM +0800, Shu Ming wrote:
On 2012-6-7 21:26, Adam Litke wrote: Yes, I agree with you. Also, we should merge this tool into vdsm as a helper for PEP8 clean work.
Thanks, Zhou Zheng! I hope this expedites the pep8 conversion process.
As the tool is not vdsm-specific, I'd rather see it in pypi.python.org than in vdsm.
+1 on pypi rather than vdsm.
On 06/11/2012 02:06 PM, Ewoud Kohl van Wijngaarden wrote:
On Sun, Jun 10, 2012 at 11:15:48AM +0300, Dan Kenigsberg wrote:
On Thu, Jun 07, 2012 at 11:13:14PM +0800, Shu Ming wrote:
On 2012-6-7 21:26, Adam Litke wrote: Yes, I agree with you. Also, we should merge this tool into vdsm as a helper for PEP8 clean work.
Thanks, Zhou Zheng! I hope this expedites the pep8 conversion process.
As the tool is not vdsm-specific, I'd rather see it in pypi.python.org than in vdsm.
+1 on pypi rather than vdsm.
+1 from me too :)
There are still some sudo calls in vdsm, mostly calls misc.exec with sudo=True. Instead of adding interfaces for each call, how about changing the misc.exec() to let it contact supervdsm when sudo=True? This could leave existing sudo call code unchanged. I wonder why supervdsm exposes many small functions, maybe a function like supervdsm.exec() may solve all? If the root action need to be limitted, may be we can give a command list check in supervdsm.exec() to restrict vdsm's behavior.
Wenchao Xia
On Wed, Jun 13, 2012 at 04:35:19PM +0800, xiaxia347os wrote:
There are still some sudo calls in vdsm, mostly calls misc.exec with sudo=True. Instead of adding interfaces for each call, how about changing the misc.exec() to let it contact supervdsm when sudo=True? This could leave existing sudo call code unchanged. I wonder why supervdsm exposes many small functions, maybe a function like supervdsm.exec() may solve all? If the root action need to be limitted, may be we can give a command list check in supervdsm.exec() to restrict vdsm's behavior.
Yes, the whole point of using supervdsm instead of sudo is to limit vdsm's root usage. Only limitted, self-contained functionality should be exposed, nothing like the
@MV_PATH@ /etc/multipath.conf *
we currently have in our sudoers.d/vdsm. The obvious implementation is a function-per-use-case. If you have another idea, that maintains security while avoiding repetitiveness, go ahead and suggest it. Nothing comes up to my mind. supervdsm.exec() with a limited set of application is not it - it is even worse than the current regexp approach of sudoer.d.
Dan.
vdsm-devel@lists.fedorahosted.org