Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None

      Description

      On default settings, Eclipse had 628 warnings. This patch removes 119 of those warnings related to unused imports. These are the safest warnings to fix and shouldn't require any testing other than confirming building still works.

      The general idea of removing warnings is both cleaner code, but also making it easier for interesting warnings to get hidden by uninteresting warnings.

        Activity

        Hide
        Paul Sundling added a comment -

        This should remove unused import eclipse warnings.

        Show
        Paul Sundling added a comment - This should remove unused import eclipse warnings.
        Hide
        Otis Gospodnetic added a comment -

        Does Eclipse take into account class references in javadocs? I think those are needed as imports in order for javadocs not to issue warnings.

        Show
        Otis Gospodnetic added a comment - Does Eclipse take into account class references in javadocs? I think those are needed as imports in order for javadocs not to issue warnings.
        Hide
        Paul Sundling added a comment -

        I just ran ant clean javadoc and it had 14 errors (not bad for a code base of this size!). Although my patch touches a lot of classes, none of them were classes I touched (judging from the '>' I see in subversive eclipse plugin for files that were modified.

        I think Eclipse is smart enough and I've seen it add imports when I add annotations. I don't use @see in javadocs that often. Let me know if you see different behavior. I reverted my changes locally and still had the same 14 javadoc errors. So I'm pretty confident that I didn't introduce errors in the patch.

        Show
        Paul Sundling added a comment - I just ran ant clean javadoc and it had 14 errors (not bad for a code base of this size!). Although my patch touches a lot of classes, none of them were classes I touched (judging from the '>' I see in subversive eclipse plugin for files that were modified. I think Eclipse is smart enough and I've seen it add imports when I add annotations. I don't use @see in javadocs that often. Let me know if you see different behavior. I reverted my changes locally and still had the same 14 javadoc errors. So I'm pretty confident that I didn't introduce errors in the patch.
        Hide
        Paul Sundling added a comment -

        I just wanted to make sure there wasn't anything you needed from me.

        These sorts of patches don't age well.

        Show
        Paul Sundling added a comment - I just wanted to make sure there wasn't anything you needed from me. These sorts of patches don't age well.
        Hide
        Ryan McKinley added a comment -

        It is three click process from source > cleanup to remove the unused imports, so keeping this up to date is easy

        In general, I think it is discouraged to change code unless there is something wrong with it.... that said, I would like to see fewer warnings in eclipse. I would also like to see the @Override annotation on overridden methods.

        Unless there are objections, I will commit changes that remove unused imports and add the @Override annotation in the next few days...

        Show
        Ryan McKinley added a comment - It is three click process from source > cleanup to remove the unused imports, so keeping this up to date is easy In general, I think it is discouraged to change code unless there is something wrong with it.... that said, I would like to see fewer warnings in eclipse. I would also like to see the @Override annotation on overridden methods. Unless there are objections, I will commit changes that remove unused imports and add the @Override annotation in the next few days...
        Hide
        Hoss Man added a comment -

        > In general, I think it is discouraged to change code unless there is something wrong with it

        the general mantra i like to follow is that commits should either change code, or fix formatting – not both, because it's too hard to tell which is which.

        I personally don't think there is anything wrong with a commit where the sole purpose is to reduce warnings (either from the compiler, or from javadoc, or from bug finding tools) – the only negative is that it sometimes makes older patches harder to apply, but in the case of unused imports, i don't think that's a big deal.

        one minor personal peeve i have is editors which sort the imports when removing the unused ones – it makes the diffs a lot more confusing then ones with simple removals

        (also: i personally don't think alphabetized import lists are as useful as import lists where related classes in the same packages (or related packages in the same namespace) are listed next to each other – but that's my own personal issue, and not an ideologue i would try and force on anyone else.)

        Show
        Hoss Man added a comment - > In general, I think it is discouraged to change code unless there is something wrong with it the general mantra i like to follow is that commits should either change code, or fix formatting – not both, because it's too hard to tell which is which. I personally don't think there is anything wrong with a commit where the sole purpose is to reduce warnings (either from the compiler, or from javadoc, or from bug finding tools) – the only negative is that it sometimes makes older patches harder to apply, but in the case of unused imports, i don't think that's a big deal. one minor personal peeve i have is editors which sort the imports when removing the unused ones – it makes the diffs a lot more confusing then ones with simple removals (also: i personally don't think alphabetized import lists are as useful as import lists where related classes in the same packages (or related packages in the same namespace) are listed next to each other – but that's my own personal issue, and not an ideologue i would try and force on anyone else.)
        Hide
        Ryan McKinley added a comment -

        I considered adding the @Override to most things, but I don't want to make applying SOLR-215 more difficult, so I think we should wait.

        Show
        Ryan McKinley added a comment - I considered adding the @Override to most things, but I don't want to make applying SOLR-215 more difficult, so I think we should wait.

          People

          • Assignee:
            Ryan McKinley
            Reporter:
            Paul Sundling
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development