Skip to content

[java] Deprecate old symbol table, add replacement for TypeHelper#2721

Merged
adangel merged 5 commits into
pmd:masterfrom
oowekyala:deprecate-old-symtable
Aug 23, 2020
Merged

[java] Deprecate old symbol table, add replacement for TypeHelper#2721
adangel merged 5 commits into
pmd:masterfrom
oowekyala:deprecate-old-symtable

Conversation

@oowekyala

Copy link
Copy Markdown
Member

Describe the PR

  • For the symbol table, only classes that were clearly internal API are deprecated. Other classes, like scope implementations, are still the only way to get a name declaration. So I don't think we should deprecate them immediately on master
  • The replacement of TypeHelper is in the package, that [java] New type resolution #2689 uses, so that it is forward-compatible
    • Many methods are not replaced. I only kept isA, and isExactlyA. Stuff like isExactlyNone, isEither, are very specific and barely used in the codebase. I'd rather have 4 public methods with a solid contract than 10 loosely consistent ones.
    • The new isA/isExactlyA have changed a bit:
      • They take the node as the second parameter. This is because usually, the longer expression is the one that produces the node (the type is usually a class literal). I think isA(String.class, someNode.getTypeNode()) reads better, even when the second expression grows longer
      • They return false if the node is null. This simplifies calling code, which usually needed a null check, and declared a temporary variable to do so. The previous behavior was mostly to throw if the node was null, though that wasn't specified, and was probably not consistent (I didn't check)
      • They throw if the class argument is null. This isn't really needed, just that I think it's also the better default, and it simplifies the implementation. The previous behavior was to return false, or throw in some cases, inconsistently. This wasn't specified.

The old methods of TypeHelper keep the same behavior but are deprecated, and implemented using TypeTestUtil

I think this is necessary before #2689 is merged

Related issues

Ready?

  • Added unit tests for fixed bug/feature
  • Passing all unit tests
  • Complete build ./mvnw clean verify passes (checked automatically by travis)
  • Added (in-code) documentation (if needed)

@oowekyala oowekyala added the is:deprecation The main focus is deprecating public APIs or rules, eg to make them internal, or removing them label Aug 22, 2020
@ghost

ghost commented Aug 22, 2020

Copy link
Copy Markdown
1 Message
📖 This changeset introduces 0 new violations, 1 new errors and 0 new configuration errors,
removes 7 violations, 1 errors and 0 configuration errors.
Full report

Generated by 🚫 Danger

@adangel adangel added this to the 6.27.0 milestone Aug 23, 2020
@adangel adangel self-assigned this Aug 23, 2020
@adangel adangel merged commit a98f3d6 into pmd:master Aug 23, 2020
@oowekyala oowekyala deleted the deprecate-old-symtable branch August 23, 2020 13:34
oowekyala added a commit that referenced this pull request Aug 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

is:deprecation The main focus is deprecating public APIs or rules, eg to make them internal, or removing them

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants