Code style questions

[Ted M. Young [@jitterted]]

I was looking through Iterables.java and found some things that seemed off:

1. The last two methods in the class were written as 'static public ...' instead of 'public static'. I assume the latter is correct? Do we/should we run CheckStyle to ensure these don't occur elsewhere?

2. I haven't (yet) figured out how to use the Eclipse formatting preferences inside of IDEA (are there instructions somewhere?), but is there a defined order for import statements (I didn't see anything in the XML file)? I have IDEA set up to reformat/reorder imports, but I don't want to go against the standard ordering.

3. The JavaDoc has duplicate @throws entries, e.g.:

   * @throws AssertionError if the given {@code Iterable} is {@code null}.
   * @throws AssertionError if the given {@code Iterable} contains any of the given values.

instead of, say,

   * @throws AssertionError if the given {@code Iterable} is {@code null} or contains any of the given values.

while I prefer the former, it's "technically" not allowed (or at least IDEA's JavaDoc inspector doesn't like it). Can I assume this is the desired and standard way to specify multiple reasons for the same thrown error/exception?

4. Spaces at the end of a line: it looks like the JavaDoc has spaces at the end some lines, e.g.:

  /**
   * Asserts that the given {@code Iterable} does not have duplicate values.
   * 
    ^ space here

I ask because I have IDEA strip spaces at the end of a line before it saves. If it'd cause merge problems and/or we want the extra space there, then I'll tell IDEA to stop doing that.


I will also edit the contributor guidelines to add this info.

;ted
--
Ted M. Young
http://about.me/tedmyoung

[Alex Ruiz]

Hey Ted,

Comments about your comments:

1. You are absolutely right. There are many inconsistencies in the
code base. I'm cleaning it up. I'm also making some fundamental
changes in the project, which is slowing down development of the rest
of the team, but I think it should be done before we release 2.0. For
example (and I mentioned earlier) we only chain methods that are
assertions. I'm also reducing the number of assertions (some that I
found not really of common use.) We'll revisit what should be added
back in 2.0.1.

2. I don't know either other than...use Eclipse? just kidding :)

3. Mea culpa. Eclipse does not enforce this and it looks like javac
doesn't (I may be wrong). Is this an IDEA-only thing? I don't have any
strong opinions. I'm happy to follow the IDEA model.

4. I absolutely hate trailing whitespace with vengeance. I have set a
"save action" in Eclipse to automatically remove any.

It would be great if you could please update the contributor guidelines.

Cheers!
-Alex

> --
> You received this message because you are subscribed to the Google Groups
> "easytesting-dev" group.
> To post to this group, send email to easytes...@googlegroups.com.
> To unsubscribe from this group, send email to
> easytesting-d...@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>
>

[Ted M. Young [@jitterted]]

OK, I'll update the contributor guidelines.

On Fri, Oct 26, 2012 at 9:42 PM, Alex Ruiz <alex.r...@gmail.com> wrote:

Hey Ted,

Comments about your comments:

1. You are absolutely right. There are many inconsistencies in the
code base. I'm cleaning it up. I'm also making some fundamental
changes in the project, which is slowing down development of the rest
of the team, but I think it should be done before we release 2.0. For
example (and I mentioned earlier) we only chain methods that are
assertions. I'm also reducing the number of assertions (some that I
found not really of common use.) We'll revisit what should be added
back in 2.0.1.

Does our CloudBees CI allow us to run CheckStyle?
 

2. I don't know either other than...use Eclipse? just kidding :)

OK, once I figure it out, I'll add that information. You didn't answer the question about order of import statements. I can use see if there's a de facto standard in the codebase and follow that. Or, we can just ignore the order and not touch it (since I always keep my import statement block collapsed, I don't really care) to avoid a huge merge headache.
 

3. Mea culpa. Eclipse does not enforce this and it looks like javac
doesn't (I may be wrong). Is this an IDEA-only thing? I don't have any
strong opinions. I'm happy to follow the IDEA model.

Looks like IDEA is wrong here, according to http://docs.oracle.com/javase/7/docs/technotes/tools/windows/javadoc.html#throws, it's allowed. I'll mention that in the contributor guidelines (and file a bug against IDEA!).
 

4. I absolutely hate trailing whitespace with vengeance. I have set a
"save action" in Eclipse to automatically remove any.

Should we run a global trailing whitespace cleanup commit?

;ted
 

[Alex Ruiz]

Sorry Ted, I missed the "order of imports" question. Here it is, from
the Eclipse preferences:

1. java
2. javax
3. org
4. com
5. java
6. javax
7. org
8. com

1,2,3,4 refer to static imports, the rest are regular imports. Note
that the current code does not follow this.

I don't know anything about CloudBees. Joel knows about it.

Global whitespace cleanup commit: Yes! please!

Cheers,
-Alex

On Sat, Oct 27, 2012 at 1:39 PM, Ted M. Young [@jitterted]

[Ted M. Young [@jitterted]]

Do we want to use wildcard imports anywhere?

Once your refactoring is complete, I can do a global whitespace/import/layout/etc. cleanup commit.

;ted

[Ted M. Young [@jitterted]]

Oh, and do we want any blank lines between the import sections?

;etd

[Ted M. Young [@jitterted]]

Two more things:

1. Do we want formatting related IDE configuration in the repo, i.e., should I commit IDEA's .idea directory (it has things like the code style, compiler settings, etc.)?
2. Is it FEST or Fest? The current README uses both, but I thought it should be FEST.

;ted

[Alex Ruiz]

We used to do wilcard imports, but I think it is not a good idea. In
my refactoring I've included explicit imports everywhere.

On Mon, Oct 29, 2012 at 5:39 PM, Ted M. Young [@jitterted]

[Alex Ruiz]

That's something that Eclipse is doing based on the order of imports
specified in preferences. I don't know how to tell Eclipse "order
imports this way but only separate static from non-static imports" :/

On Mon, Oct 29, 2012 at 5:40 PM, Ted M. Young [@jitterted]

[Alex Ruiz]

I don't know if we should commit .idea directory yet. I think we are
not storing any IDE-specific project configuration (like .classpath
and .project files) but we are storing configuration preferences. Does
the .idea directory only contain preferences and no project-specific
info?

-Alex

On Mon, Oct 29, 2012 at 6:29 PM, Ted M. Young [@jitterted]

[Alex Ruiz]

I forgot in my last reply: you are right, it should be FEST, not Fest.

On Mon, Oct 29, 2012 at 6:29 PM, Ted M. Young [@jitterted]