Dealing with the reformat for Simplename
I think you can work around a reformat this way: 1. Do a reformat of the code prior to the simplename patch and tag it 2. Do a reformat of the code with the simplename patch applied and tag it 3. Generate a patch that is the diff of steps 1 and 2 No guarantees, but it should be pretty much the same to apply the new patch on top of the reformat as on top of step 1 above.
Ade, I found that curly braces 'same line' formatting is tied to the 'never join' setting for line wrapping. If I select 'never join' it will not join the curly braces into 'same line'. If I unselect 'never join' it will join the curly braces, but it will join other stuff too, which is not what we want. I'm not sure whether this behavior is specific to my Eclipse version (3.7.1 on F16). If your Eclipse works better please go ahead with formatting the master branch and Adam's rebased patches in ticket-2 branch below: http://fedorapeople.org/gitweb?p=edewata/public_git/pki.git;a=summary Another option is to use the formatting I did in branch ticket-45-2 for now and address the curly braces problem later. Thanks. On 1/9/2012 7:44 AM, Adam Young wrote:
I think you can work around a reformat this way:
1. Do a reformat of the code prior to the simplename patch and tag it 2. Do a reformat of the code with the simplename patch applied and tag it 3. Generate a patch that is the diff of steps 1 and 2
No guarantees, but it should be pretty much the same to apply the new patch on top of the reformat as on top of step 1 above.
-- Endi S. Dewata
Endi, I was able to format correctly using my version of eclipse. I also spent some time cleaning up the most egregious formatting errors - and think I got most of them. I applied the same to adam's changes too - with only one easy merge conflict to be resolved. Ping me on #irc and I'll post for your review in the morning. Ade On Mon, 2012-01-09 at 21:21 -0600, Endi Sukma Dewata wrote:
Ade,
I found that curly braces 'same line' formatting is tied to the 'never join' setting for line wrapping. If I select 'never join' it will not join the curly braces into 'same line'. If I unselect 'never join' it will join the curly braces, but it will join other stuff too, which is not what we want.
I'm not sure whether this behavior is specific to my Eclipse version (3.7.1 on F16). If your Eclipse works better please go ahead with formatting the master branch and Adam's rebased patches in ticket-2 branch below:
http://fedorapeople.org/gitweb?p=edewata/public_git/pki.git;a=summary
Another option is to use the formatting I did in branch ticket-45-2 for now and address the curly braces problem later.
Thanks.
On 1/9/2012 7:44 AM, Adam Young wrote:
I think you can work around a reformat this way:
1. Do a reformat of the code prior to the simplename patch and tag it 2. Do a reformat of the code with the simplename patch applied and tag it 3. Generate a patch that is the diff of steps 1 and 2
No guarantees, but it should be pretty much the same to apply the new patch on top of the reformat as on top of step 1 above.
On 1/9/2012 10:56 PM, Ade Lee wrote:
I was able to format correctly using my version of eclipse. I also spent some time cleaning up the most egregious formatting errors - and think I got most of them.
I applied the same to adam's changes too - with only one easy merge conflict to be resolved.
I'm not quite done reviewing the patch, it's going to take a while to review the entire 300k line patch. It would be much better if you could fix the fedorapeople repo so that we can do the review the patch as a single page in the browser like this: http://fedorapeople.org/gitweb?p=edewata/public_git/pki.git;a=commitdiff;h=0... Another option is to split the formatting into smaller manageable patches which can be reviewed by several people. So far I found formatting problems related to comments. There are too many of them to list individually, fix, and then review again. So I think it might be better not to reformat the comments at this time. Here are some of them: 1. Some comments or commented codes are re-wrapped removing important line breaks and indentations. This happens in: * pki/base/ca/src/com/netscape/ca/CAService.java line 170 * pki/base/ca/src/com/netscape/ca/CMSCRLExtensions.java line 491 * pki/base/ca/src/com/netscape/ca/CRLIssuingPoint.java lines 1236, 1445, 1782 * pki/base/ca/src/com/netscape/ca/CertificateAuthority.java line 1656, 1729 * pki/base/common/src/com/netscape/certsrv/apps/CMS.java line 246 2. Some comments use three slashes or longer (///). The slashes are split into // and the remaining /. This happens in: * pki/base/ca/src/com/netscape/ca/CAService.java line 546 * pki/base/ca/src/com/netscape/ca/CertificateAuthority.java line 1164 3. The <PRE> tag gets additional indentation: * pki/base/common/src/com/netscape/certsrv/acls/ACLEntry.java lines 69, 82 Other than that the code builds just fine and passes the unit tests. -- Endi S. Dewata
Ok -- so I reformatted and changed a couple of things: I do as follows: 1. format with line length = 500 for code and text, no joining for code and text. Also, no formatting for line comments. 2. format with line length = 120 for code 3. format with line length = 120 for comments 4. apply patch to fix bad wraps in code. As there is no line comment formatting - this should address the issues you have observed. Its in my repo as formatting-5 for your review. ssh://vakwetu@fedorapeople.org/home/fedora/vakwetu/public_git/pki.git * [new branch] formatting-5 -> formatting-5 Thanks, Ade On Tue, 2012-01-10 at 19:13 -0600, Endi Sukma Dewata wrote:
On 1/9/2012 10:56 PM, Ade Lee wrote:
I was able to format correctly using my version of eclipse. I also spent some time cleaning up the most egregious formatting errors - and think I got most of them.
I applied the same to adam's changes too - with only one easy merge conflict to be resolved.
I'm not quite done reviewing the patch, it's going to take a while to review the entire 300k line patch. It would be much better if you could fix the fedorapeople repo so that we can do the review the patch as a single page in the browser like this:
http://fedorapeople.org/gitweb?p=edewata/public_git/pki.git;a=commitdiff;h=0...
Another option is to split the formatting into smaller manageable patches which can be reviewed by several people.
So far I found formatting problems related to comments. There are too many of them to list individually, fix, and then review again. So I think it might be better not to reformat the comments at this time. Here are some of them:
1. Some comments or commented codes are re-wrapped removing important line breaks and indentations. This happens in:
* pki/base/ca/src/com/netscape/ca/CAService.java line 170 * pki/base/ca/src/com/netscape/ca/CMSCRLExtensions.java line 491 * pki/base/ca/src/com/netscape/ca/CRLIssuingPoint.java lines 1236, 1445, 1782 * pki/base/ca/src/com/netscape/ca/CertificateAuthority.java line 1656, 1729 * pki/base/common/src/com/netscape/certsrv/apps/CMS.java line 246
2. Some comments use three slashes or longer (///). The slashes are split into // and the remaining /. This happens in: * pki/base/ca/src/com/netscape/ca/CAService.java line 546 * pki/base/ca/src/com/netscape/ca/CertificateAuthority.java line 1164
3. The <PRE> tag gets additional indentation: * pki/base/common/src/com/netscape/certsrv/acls/ACLEntry.java lines 69, 82
Other than that the code builds just fine and passes the unit tests.
After review by Nathan and Endi, and ack from Endi in #irc , pushed to master. Dropped block comment formatting. Patch also formatted and provided to Endi for ayoung patches. (ayoung-ticket-2) Ade On Wed, 2012-01-11 at 00:58 -0500, Ade Lee wrote:
Ok -- so I reformatted and changed a couple of things:
I do as follows: 1. format with line length = 500 for code and text, no joining for code and text. Also, no formatting for line comments. 2. format with line length = 120 for code 3. format with line length = 120 for comments 4. apply patch to fix bad wraps in code.
As there is no line comment formatting - this should address the issues you have observed.
Its in my repo as formatting-5 for your review.
ssh://vakwetu@fedorapeople.org/home/fedora/vakwetu/public_git/pki.git * [new branch] formatting-5 -> formatting-5
Thanks, Ade
On Tue, 2012-01-10 at 19:13 -0600, Endi Sukma Dewata wrote:
On 1/9/2012 10:56 PM, Ade Lee wrote:
I was able to format correctly using my version of eclipse. I also spent some time cleaning up the most egregious formatting errors - and think I got most of them.
I applied the same to adam's changes too - with only one easy merge conflict to be resolved.
I'm not quite done reviewing the patch, it's going to take a while to review the entire 300k line patch. It would be much better if you could fix the fedorapeople repo so that we can do the review the patch as a single page in the browser like this:
http://fedorapeople.org/gitweb?p=edewata/public_git/pki.git;a=commitdiff;h=0...
Another option is to split the formatting into smaller manageable patches which can be reviewed by several people.
So far I found formatting problems related to comments. There are too many of them to list individually, fix, and then review again. So I think it might be better not to reformat the comments at this time. Here are some of them:
1. Some comments or commented codes are re-wrapped removing important line breaks and indentations. This happens in:
* pki/base/ca/src/com/netscape/ca/CAService.java line 170 * pki/base/ca/src/com/netscape/ca/CMSCRLExtensions.java line 491 * pki/base/ca/src/com/netscape/ca/CRLIssuingPoint.java lines 1236, 1445, 1782 * pki/base/ca/src/com/netscape/ca/CertificateAuthority.java line 1656, 1729 * pki/base/common/src/com/netscape/certsrv/apps/CMS.java line 246
2. Some comments use three slashes or longer (///). The slashes are split into // and the remaining /. This happens in: * pki/base/ca/src/com/netscape/ca/CAService.java line 546 * pki/base/ca/src/com/netscape/ca/CertificateAuthority.java line 1164
3. The <PRE> tag gets additional indentation: * pki/base/common/src/com/netscape/certsrv/acls/ACLEntry.java lines 69, 82
Other than that the code builds just fine and passes the unit tests.
_______________________________________________ Pki-devel mailing list Pki-devel@redhat.com https://www.redhat.com/mailman/listinfo/pki-devel
participants (3)
-
Adam Young -
Ade Lee -
Endi Sukma Dewata