Details

    • Type: Task Task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 3.0
    • Component/s: None
    • Labels:
      None
    • Lucene Fields:
      New, Patch Available

      Description

      During removal of deprecated APIs, mostly the problem was, to not only remove the method in the (abstract) base class (e.g. Scorer.explain()), but also remove it in sub classes that override it. You can easily forget that (especially, if the method was not marked deprecated in the subclass). By adding @Override annotations everywhere in Lucene, such removals are simple, because the compiler throws out an error message in all subclasses which then no longer override the method.

      Also it helps preventing the well-known traps like overriding hashcode() instead of hashCode().

      The patch was generated automatically, and is rather large. Should I apply it, or would it break too many patches (but I think, trunk has changed so much, that this is only a minimum of additional work to merge)?

      1. LUCENE-2012.patch
        288 kB
        Uwe Schindler
      2. LUCENE-2012-tests.patch
        164 kB
        Uwe Schindler
      3. LUCENE-2012_contrib.patch
        344 kB
        Robert Muir
      4. LUCENE-2012_contrib.patch
        332 kB
        Robert Muir

        Activity

        Hide
        Uwe Schindler added a comment -

        Yes, that was exactly the problem, but it is not a bug, it simply replaced the incorrect indended line by an @Override, followed by a new line and then the correct indended method declaration. This lead to the fact, that the second line with the method declaration was looking ugly.

        Show
        Uwe Schindler added a comment - Yes, that was exactly the problem, but it is not a bug, it simply replaced the incorrect indended line by an @Override, followed by a new line and then the correct indended method declaration. This lead to the fact, that the second line with the method declaration was looking ugly.
        Hide
        Robert Muir added a comment -

        Uwe, just curious if you had this problem? when the files had funky formatting eclipse would sometimes insert the @override with the "correct" (existing) indentation, but would change the formatting of the method itself... seems like a bug... i corrected this and saw you had a similar comment...

        Show
        Robert Muir added a comment - Uwe, just curious if you had this problem? when the files had funky formatting eclipse would sometimes insert the @override with the "correct" (existing) indentation, but would change the formatting of the method itself... seems like a bug... i corrected this and saw you had a similar comment...
        Hide
        Uwe Schindler added a comment -

        Thanks Robert! I was in plane and wasn't able to do heavy committing!

        Show
        Uwe Schindler added a comment - Thanks Robert! I was in plane and wasn't able to do heavy committing!
        Hide
        Robert Muir added a comment -

        Committed revision 833867. (contrib, contrib tests, and demo)

        Show
        Robert Muir added a comment - Committed revision 833867. (contrib, contrib tests, and demo)
        Hide
        Robert Muir added a comment -

        there were some places where @override was added for generated code (snowball, javacc), I excluded these files, as was done for core.

        also add @override for demo. will commit and resolve this soon as this will quickly get out of date.

        Show
        Robert Muir added a comment - there were some places where @override was added for generated code (snowball, javacc), I excluded these files, as was done for core. also add @override for demo. will commit and resolve this soon as this will quickly get out of date.
        Hide
        Robert Muir added a comment -

        Uwe attached is a patch for contrib and its tests.

        Show
        Robert Muir added a comment - Uwe attached is a patch for contrib and its tests.
        Hide
        Uwe Schindler added a comment -

        Committed revision: 832972

        Show
        Uwe Schindler added a comment - Committed revision: 832972
        Hide
        Uwe Schindler added a comment - - edited

        test for the patches, it also fixes whitespace where broken. EDIT other way round: patch for the tests

        Will commit now.

        Show
        Uwe Schindler added a comment - - edited test for the patches, it also fixes whitespace where broken. EDIT other way round: patch for the tests Will commit now.
        Hide
        Mark Miller added a comment - - edited

        What about tests and contrib Uwe? Shouldn't we just hit them all?

        edit

        scratch that - sorry - missed your comment that you will do contrib/tests next

        Show
        Mark Miller added a comment - - edited What about tests and contrib Uwe? Shouldn't we just hit them all? edit scratch that - sorry - missed your comment that you will do contrib/tests next
        Hide
        Uwe Schindler added a comment -

        No problem But then it should also have stopped at generics :-]

        Show
        Uwe Schindler added a comment - No problem But then it should also have stopped at generics :-]
        Hide
        Cédrik LIME added a comment -

        Oops, my mistake, sorry.
        'twas my workspace I didn't upgrade from 1.4...
        Mea culpa!

        Show
        Cédrik LIME added a comment - Oops, my mistake, sorry. 'twas my workspace I didn't upgrade from 1.4... Mea culpa!
        Hide
        Uwe Schindler added a comment -

        There are no @Overrides on interfaces.

        By the way, I always-ever use 1.5. I do not have any shitty 1.6 on my computer

        Show
        Uwe Schindler added a comment - There are no @Overrides on interfaces. By the way, I always-ever use 1.5. I do not have any shitty 1.6 on my computer
        Hide
        Cédrik LIME added a comment -

        This patch breaks compatibility with Java 5!
        @Override annotations are only available on interface implementation from Java 1.6

        Show
        Cédrik LIME added a comment - This patch breaks compatibility with Java 5! @Override annotations are only available on interface implementation from Java 1.6
        Hide
        Uwe Schindler added a comment -

        Committed trunk changes in revision: 830661

        I will proceed with contrib and test later (not yet).

        Show
        Uwe Schindler added a comment - Committed trunk changes in revision: 830661 I will proceed with contrib and test later (not yet).
        Hide
        Uwe Schindler added a comment -

        If nobody objects, I will commit during the day. And then do the same with tests/contrib.

        Show
        Uwe Schindler added a comment - If nobody objects, I will commit during the day. And then do the same with tests/contrib.
        Hide
        Earwin Burrfoot added a comment -

        That's why you need @override in first place - any decent Java IDE shows that method overrides/has overrides.

        Show
        Earwin Burrfoot added a comment - That's why you need @override in first place - any decent Java IDE shows that method overrides/has overrides.
        Hide
        Uwe Schindler added a comment -

        I use normally only my text editor for developing, but for such a thing, Eclipse is good:
        Right mouse on src/java folder, Source -> Cleanup - Use Custom Profile -> remove everything except "add missing @Override"

        Show
        Uwe Schindler added a comment - I use normally only my text editor for developing, but for such a thing, Eclipse is good: Right mouse on src/java folder, Source -> Cleanup - Use Custom Profile -> remove everything except "add missing @Override"
        Hide
        DM Smith added a comment -

        Uwe, what did you use to generate the @override?

        Show
        DM Smith added a comment - Uwe, what did you use to generate the @override?
        Hide
        Mark Miller added a comment -

        +1 - I've felt this pain.

        Also, in keeping the flex branch up to date, I've seen that
        the recent trurn has been incredible. Addng this to the mix
        isn't going to put things over any tipping point IMO. If there is a patch
        out there that hasn't been severely affected recently, it's small
        enough not to matter - and I've swalled so many merges on the flex
        branch that I don't much care about swallowing this patch too.
        Easy merging with this one

        Show
        Mark Miller added a comment - +1 - I've felt this pain. Also, in keeping the flex branch up to date, I've seen that the recent trurn has been incredible. Addng this to the mix isn't going to put things over any tipping point IMO. If there is a patch out there that hasn't been severely affected recently, it's small enough not to matter - and I've swalled so many merges on the flex branch that I don't much care about swallowing this patch too. Easy merging with this one
        Hide
        Uwe Schindler added a comment -

        Patch.

        Show
        Uwe Schindler added a comment - Patch.

          People

          • Assignee:
            Uwe Schindler
            Reporter:
            Uwe Schindler
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development