Constants are not constant in the core.

[GoogleCodeExporter]

I've pushed the CheckStyle number as far down as I can without breaking
binary incompatibility. The next stumbling block is the "constants" that
aren't declared as final. Considering the overall quality of the
framework's codebase lately, this is an oversight that stands out like a
sore thumb.

It has to have been an error (albeit a small one) at the time. So we
shouldn't propagate it forward. I can't think of a situation where this
oversight would be used by a developer without them being aware that they
shouldn't be allowed to be doing what they're doing; heck even the naming
convention makes them not want to change the value. If we mark them as
final now, no future developers could make use of this hole.

I'm 100% for breaking compatibility in these cases.

Anyone else?

Original issue reported on code.google.com by allain.lalonde on 13 Oct 2009 at 4:55

[GoogleCodeExporter]

agreed

Original comment by mr0...@mro.name on 13 Oct 2009 at 5:02

Attachments:

• +1_star.png

[GoogleCodeExporter]

I'm fully for these changes, but would rather they wait for 2.0.  Changes that 
break
binary compatibility should only happen at major release number increases.

Original comment by heue...@gmail.com on 13 Oct 2009 at 5:14

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

Breaking binary compatibility is only a problem in this particular case if the 
user
of the library is changing the value of constants. I'd almost bet money that 
this
won't break any client code. 

If we were changing method signatures, I'd absolutely push for 2.0, but this 
isn't
that. The only user story this would break is the deviant one. I'm all for that.

Original comment by allain.lalonde on 13 Oct 2009 at 5:43

[GoogleCodeExporter]

> I'd almost bet money that this won't break any client code. 

Agreed, and if it did, the client would simply receive an IllegalAccessException
where they tried to set the value of the static final field.

I think of it as more of a marketing thing.  You can't say your bread is 
certified
organic if you put any one non-organic ingredient in it.  Likewise, we can't 
claim
1.3 is binary compatible with 1.2.1 if we break even once.

Original comment by heue...@gmail.com on 13 Oct 2009 at 6:43

[GoogleCodeExporter]

Funny. I'd be willing to replace certified binary compatible, to certified not 
to
brake sane client code :)

Original comment by allain.lalonde on 13 Oct 2009 at 6:50

[GoogleCodeExporter]

I completely go with michael when it comes to "stick to the rules".

BUT.

Mind the frequency of our releases. When do we expect to release a new major?

Is agreed-on ill-written (most possibly not) existing client code really so 
dear to us, that we'll rather not make 
significant style improvements NOW?

I'd say no and make 'em final, feeling even less happy with the other option.

Original comment by mr0...@mro.name on 13 Oct 2009 at 7:04

[GoogleCodeExporter]

And: getting meaningful checkstyle reports, namely ones with near zero 
complaints, has a value by itself.

I see this as crucial for using such tools at all.

But don't get me wrong - it's not checkstyle against binary comp. It's 
conserving bad coding errors vs. binary 
comp.

Original comment by mr0...@mro.name on 13 Oct 2009 at 7:13

[GoogleCodeExporter]

A workaround that retains binary compatibility would be to

1) create new static final fields where necessary
2) re-implement internal code to use only the new fields
3) mark the static non-final fields as @deprecated in version 1.3
4) remove the static non-final fields in version 2.0

The problem with this approach is that there are a lot of these, and we'd be 
making
the API uglier before we would be able to make it nicer.

I vote for doing nothing in version 1.3 and making them all final in version 
2.0.

Original comment by heue...@gmail.com on 21 Oct 2009 at 2:23

[GoogleCodeExporter]

Seconded and done.

Original comment by allain.lalonde on 21 Oct 2009 at 2:42

• Added labels: Milestone-2.0
• Removed labels: Milestone-1.3

[GoogleCodeExporter]

Fixed in r935

Original comment by allain.lalonde on 19 Jan 2010 at 9:09

[GoogleCodeExporter]

Original comment by allain.lalonde on 19 Jan 2010 at 9:13

• Changed state: Fixed

[GoogleCodeExporter]

Moving status back to Accepted, needs to be done on trunk.

Original comment by heue...@gmail.com on 9 Apr 2010 at 9:43

• Changed state: Accepted

[GoogleCodeExporter]

Commit #1003 adds the final keyword where appropriate.

Original comment by allain.lalonde on 16 Apr 2010 at 1:49

• Changed state: Fixed

[GoogleCodeExporter]

Original comment by heue...@gmail.com on 2 Mar 2011 at 4:10

• Changed state: Verified