Constants are not constant in the core.

[mro]

Originally reported on Google Code with ID 135

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?

Reported by allain.lalonde on 2009-10-13 16:55:34

[mro]

agreed

Reported by mr0738@mro.name on 2009-10-13 17:02:25

---

• _Attachment: +1_star.png
  _

[mro]

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.

Reported by heuermh on 2009-10-13 17:14:28

[mro]

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.

Reported by allain.lalonde on 2009-10-13 17:43:57

[mro]

> 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.

Reported by heuermh on 2009-10-13 18:43:45

[mro]

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

Reported by allain.lalonde on 2009-10-13 18:50:28

[mro]

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.

Reported by mr0738@mro.name on 2009-10-13 19:04:03

[mro]

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.

Reported by mr0738@mro.name on 2009-10-13 19:13:47

[mro]

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.

Reported by heuermh on 2009-10-21 02:23:36

[mro]

Seconded and done.

Reported by allain.lalonde on 2009-10-21 02:42:50

• Labels added: Milestone-2.0
• Labels removed: Milestone-1.3

[mro]

Fixed in r935

Reported by allain.lalonde on 2010-01-19 21:09:41

[mro]

Reported by allain.lalonde on 2010-01-19 21:13:31

• Status changed: Fixed

[mro]

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

Reported by heuermh on 2010-04-09 21:43:32

• Status changed: Accepted

[mro]

Commit #1003 adds the final keyword where appropriate.

Reported by allain.lalonde on 2010-04-16 01:49:19

• Status changed: Fixed

[mro]

Reported by heuermh on 2011-03-02 16:10:21

• Status changed: Verified