search

neo09 / gwt-platform

Automatically exported from code.google.com/p/gwt-platform
0 stars 0 forks source link

Document coding standards and use Checkstyle #82

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago 
At the moment the coding standards are:

- Use 2 spaces. No tabs.

There seems to be inconsistent use of spaces after opening brackets and 
before closing brackets?

Do most other people use eclipse? Perhaps we could provide the code 
formatting template somewhere so all code would have consistent formatting.

Original issue reported on code.google.com by brendanp...@gmail.com on 3 May 2010 at 5:08

GoogleCodeExporter commented 9 years ago 
Sometimes its (abc != def), ( abc != def) or ( abc != def )

Personally I like the rule of always use braces, even on single line ifs, as 
it's 
safer.

Original comment by brendanp...@gmail.com on 3 May 2010 at 5:09

GoogleCodeExporter commented 9 years ago 
You're right, I haven't been extra careful on the coding style. It's probably 
worth 
setting up a standard and doing an overall clean-up pass.

I suggest we go with the standard used in GWT, which is close enough to what 
I'm 
doing now:
http://code.google.com/webtoolkit/makinggwtbetter.html#codestyle

If contributors are alright with this, I'll document it in the wiki and we can 
open 
an issue for porting the current code to this standard.

Original comment by philippe.beaudoin on 3 May 2010 at 5:59

GoogleCodeExporter commented 9 years ago 
[deleted comment]
GoogleCodeExporter commented 9 years ago 
Bumping up, preparing for release.

Original comment by philippe.beaudoin on 13 May 2010 at 9:49

GoogleCodeExporter commented 9 years ago 
Realistically, is this ever going to happen? :)  Bumping to 0.4.

Original comment by philippe.beaudoin on 28 Jun 2010 at 5:58

GoogleCodeExporter commented 9 years ago 
I think the real way to make this happen is to start using checkStyle and to 
build a checkStyle descriptor... If anybody is up to the task, this would be 
super nice!
http://checkstyle.sourceforge.net/

Original comment by philippe.beaudoin on 6 Jul 2010 at 7:41

GoogleCodeExporter commented 9 years ago

Original comment by philippe.beaudoin on 6 Jul 2010 at 7:41

GoogleCodeExporter commented 9 years ago 
GWT has a clearly defined code style.  
http://code.google.com/webtoolkit/makinggwtbetter.html#codestyle

gwt-platform should implement the same coding standard as GWT.

The code style and formatting rules are under 
http://google-web-toolkit.googlecode.com/svn/trunk/eclipse/ - mostly under 
settings.

http://codereview.appspot.com/1867045/show

Original comment by brendanp...@gmail.com on 26 Jul 2010 at 11:12

GoogleCodeExporter commented 9 years ago 
Cool, I'll check out those style for myself, it'll be a start ! I'll do a 
codereview after trying it.

GJ Brendan !

Original comment by goudreau...@gmail.com on 26 Jul 2010 at 11:17

GoogleCodeExporter commented 9 years ago 
There's a Eclipse CS plugin style descriptor inside the code style folder and 
it is not compatible with the latest version of CheckStyle ! ...

Original comment by goudreau...@gmail.com on 26 Jul 2010 at 1:32

GoogleCodeExporter commented 9 years ago 
I know.  I've only taken rules that has made it into gwt trunk.

The instructions in eclipse/README.txt say to use the 4.4.2 version (4.4.3 
works for me).

I've starred this bug, and was planning to update to 5.x when this issue is 
completed.
http://code.google.com/p/google-web-toolkit/issues/detail?id=3828

We could use the files attached to the issue instead, but then we have deviated 
from gwt.

Original comment by brendanp...@gmail.com on 26 Jul 2010 at 9:45

GoogleCodeExporter commented 9 years ago 
I personnaly prefer latest version, at least, when GWT will be updated, 
everything will work even if we took a step ahead. Anyway, everything is 
working great and importing the xml files is a piece of cake.

Original comment by goudreau...@gmail.com on 26 Jul 2010 at 9:56

GoogleCodeExporter commented 9 years ago 
One more comment, it's not like codding standard will change a lot until google 
get up to date with cs.

Original comment by goudreau...@gmail.com on 26 Jul 2010 at 9:59

GoogleCodeExporter commented 9 years ago 
Ok.  I'll update to latest version and see what happens.

I fixed most of the rules that checkstyle was complaining about. 
I ignored the one about missing javadoc comments (because I didn't know what to 
put), and the one about methods not in alphabetical order (it would make the 
patch super large).

Do we want to force our methods to be in alphabetical order (which is the gwt 
standard)?

Original comment by brendanp...@gmail.com on 26 Jul 2010 at 10:37

GoogleCodeExporter commented 9 years ago 
Good question, personnally I'd go for it simply because it's a nice to have the 
same structure for everyone that works on the project.

I'd love to hear more comments about this before taking any decision anyway.

Original comment by goudreau...@gmail.com on 26 Jul 2010 at 10:43

GoogleCodeExporter commented 9 years ago 
I want run the method sorting by everyone else before the changes are made.

This is the gwt method sorting
http://google-web-toolkit.googlecode.com/svn/trunk/eclipse/settings/code-style/g
wt-sort-order.png + alphabetical.

Is everyone happy with that.

Original comment by brendanp...@gmail.com on 26 Jul 2010 at 10:45

GoogleCodeExporter commented 9 years ago 
New review using the 5.x checkstyle eclipse plugin. 
http://codereview.appspot.com/1878045/show

However I didn't manage to get the com.google.gwt.checkstyle_1.0.0.jar working. 
Perhaps I'm putting it in the wrong place.

Christian, do you want to give it a try?  
I've uploaded the changes to a clone at 
http://code.google.com/r/brendanpdoherty-checkstyle/source/list

Original comment by brendanp...@gmail.com on 27 Jul 2010 at 7:39

GoogleCodeExporter commented 9 years ago 
It works, but I imported the entire project into workspace. There must be a way 
to use the Jar, but you need to register it somewhere... I'll take a more 
deeper look and try to use the jar, in the meanwhile importing the project let 
you use the custom checks.

Here's more information.

http://eclipse-cs.sourceforge.net/extending_custom_checks.html

Cheers,

Original comment by goudreau...@gmail.com on 27 Jul 2010 at 1:27

GoogleCodeExporter commented 9 years ago 
[deleted comment]
GoogleCodeExporter commented 9 years ago 
[deleted comment]
GoogleCodeExporter commented 9 years ago 
I removed a check for @author tags since I think this is something that we 
need. I use it a lot, Gwt-Platform use this a lot, and I don't think we should 
throw an error while using it.

Anyway, I don't really understand why there's a check/error on that.

Original comment by goudreau...@gmail.com on 27 Jul 2010 at 1:47

Attachments:

GoogleCodeExporter commented 9 years ago 
Great. Maybe the GWT project doesn't want @author tags everywhere. However, I 
think that for Gwt-Platform, I like the attribution aspect of it, especially as 
we have forked a number of different projects.

Original comment by brendanp...@gmail.com on 27 Jul 2010 at 9:16

GoogleCodeExporter commented 9 years ago 
I have some additional changes I would like to submit, but have time 
constraints.  

Will probably submit changes in about a week.

Original comment by brendanp...@gmail.com on 3 Aug 2010 at 11:37

GoogleCodeExporter commented 9 years ago 
Is the custom check working for you ?

Anyway LGTM, but you'll have to tell how exactly using the CS plugin with the 
Custom Check.

Original comment by goudreau...@gmail.com on 14 Aug 2010 at 2:32

GoogleCodeExporter commented 9 years ago 
Brendan, Christian, thanks so much for your work on this!

I'm coming late to this issue but am interested in using checkstyle on my build 
(and asking every commiter to do the same). At some point we will setup an 
automatic build process including the CheckStyle, see Issue 162.

So:
1) Is there a version of the source code with corrected style? If so, let's 
commit it as soon as possible so that we can start on the right track.
2) Can someone write a short wiki page for developers to explain how to setup 
and checkstyle, and pointing to the guideline we use?

Original comment by philippe.beaudoin on 14 Aug 2010 at 5:12

GoogleCodeExporter commented 9 years ago 
Here is a patch to review. http://codereview.appspot.com/1947045

To see it in action, 
1. Install http://eclipse-cs.sourceforge.net/
2. Check out https://brendanpdoherty-checkstyle2.googlecode.com/hg/  

That's it.

Checkstyle raises 1000's of issues, so I think it's best to fix them in several 
seperate patch.

Original comment by brendanp...@gmail.com on 15 Aug 2010 at 11:42

GoogleCodeExporter commented 9 years ago 
Bah, it's only formatting issue, it doesn't really matter to do it at once.

Original comment by goudreau...@gmail.com on 15 Aug 2010 at 12:52

GoogleCodeExporter commented 9 years ago 
Excellent, I'll pull the patch. 

Anybody willing to write a "contributor guideline" wiki? For the moment I'm 
fine with it only containing the checkstyle guidelines...

Original comment by philippe.beaudoin on 15 Aug 2010 at 5:09

GoogleCodeExporter commented 9 years ago 
Have a read of this.
http://code.google.com/p/gwt-platform/wiki/ContributorGuidelines

Philippe, can you add something to it about the guiding principles behind gwtp.

Original comment by brendanp...@gmail.com on 16 Aug 2010 at 12:40

GoogleCodeExporter commented 9 years ago 
Thanks Brendan for the coding guidelines, I'll see if I can update it. I pulled 
the patch into the trunk. Things left before that issue can be closed:

1) Proofread the wiki and make sure it includes the documentation on how the 
contributors can run checkstyle locally.
2) Get rid of all checkstyle issues.

If someone tackles 2, feel free to do it right in the trunk. Please do it in 
small commits (of a few files) so we can rollback only part of it if things go 
wrong.

Original comment by philippe.beaudoin on 16 Aug 2010 at 6:08

GoogleCodeExporter commented 9 years ago 
Just to let you know brendan that I'll do 2) Get rid of all checkstyle issues.

Feel free to double check me after committing the change.

Original comment by goudreau...@gmail.com on 16 Aug 2010 at 6:14

GoogleCodeExporter commented 9 years ago

Original comment by goudreau...@gmail.com on 16 Aug 2010 at 7:49

GoogleCodeExporter commented 9 years ago 
165 is done, is there anything else to this issue or can we delete it ?

Original comment by goudreau...@gmail.com on 25 Aug 2010 at 12:45

GoogleCodeExporter commented 9 years ago 
I just want to slightly update the wiki before we close this. I'll do it soon.

*Things left to be done:*
1) Proofread the wiki and make sure it includes the documentation on how the 
contributors can run checkstyle locally.

Original comment by philippe.beaudoin on 25 Aug 2010 at 2:50

GoogleCodeExporter commented 9 years ago 
Proofread done, closing the issue. Thanks! This checkstyle configuration has 
proven super useful!

Original comment by philippe.beaudoin on 2 Sep 2010 at 10:29