Better criteria for public/private/protected methods

[MaKleSoft]

From https://github.com/Polymer/polymer-analyzer/blob/master/src/polymer/js-utils.ts#L113:

if (explicitPrivacy) {
    return explicitPrivacy;
  } else if (specificName.startsWith('__')) {
    return 'private';
  } else if (specificName.startsWith('_')) {
    return 'protected';
  } else if (specificName.endsWith('_')) {
    return 'private';
  } else if (configurationProperties.has(specificName)) {
    return 'protected';
  } else {
    if (privateUnlessDocumented) {
      // Some members, like methods or properties on classes are private by
      // default unless they have documentation.
      const hasDocs = !!annotation && !jsdoc.isAnnotationEmpty(annotation);
      return hasDocs ? 'public' : 'private';
    } else {
      // Other members, like entries in the Polymer `properties` block are
      // public unless there are clear signals otherwise.
      return 'public';
    }
  }

The whole idea of 'private unless documented' doesn't make a lot of sense to me. We're using two different sources of information to infer privacy.

1. Extract privacy from jsdocs
2. Infer privacy from method names (_protected, __private etc.)

Explicit privacy annotation via jsdoc trumps method naming, which makes sense, but why are we assuming undocumented methods are private (I know this is optional, but it's what we're using for scanning Polymer elements, for example). And why does this rule only apply to 'regular' named methods and not to pre/suffixed ones? this has the weird effect that an undocumented, 'regular' method has a higher privacy than a prefixed one:

myMethod() {} // --> private,
_otherMethod() {} // --> protected

This just seems wildly inconsistent to me. Another weird thing is that add any comment to a method makes it public.

// TODO: fixme
myMethod() {} // --> public

I think if we infer privacy from method names, we should be consistent about it. I.e. in the absence of an explicit jsdoc privacy annotation:

• regularMethod() {}: public
• _prefixed() {}: protected
• __doublePrefixed() {}, postFixed_(): private

Optionally we can opt out of inferring privacy from method names altogether and instead assume a default privacy, but then we should do this consistently for all kinds of methods, not just for 'regular' ones.

Here is my proposed patch:

export function getOrInferPrivacy(
    name: string,
    annotation: jsdoc.Annotation|undefined,
    inferFromName = true,
    defaultPrivacy: Privacy = 'private'): Privacy {
  const explicitPrivacy = jsdoc.getPrivacy(annotation);
  const specificName = name.slice(name.lastIndexOf('.') + 1);

  if (explicitPrivacy) {
    return explicitPrivacy;
  } else if (inferFromName) {
    if (specificName.startsWith('__') || specificName.endsWith('_')) {
      return 'private';
    } else if (specificName.startsWith('_') || configurationProperties.has(specificName)) {
      return 'protected';
    } else {
      return 'public';
    }
  } else {
    return defaultPrivacy;
  }
}

Thoughts? I'm happy to put up a PR if you guys agree with the changes.

[rictic]

@justinfagnani what do you think? We went with private unless documented mostly for the documentation case, but in hindsight I think that's premature, or at least overspecialized to polymer core. Better for documentation viewer to handle that I think.

[justinfagnani]

I think the suggestion here sounds reasonable. I slated it for 2.0, but it might not be blocking depending on the amount of time we have. It'd be nice to do.

[MaKleSoft]

Like I said, I'm happy to take a shot at it if you want.

[rictic]

Go for it!

[MaKleSoft]

Ok, PR is out. Sorry for taking so long!

[justinfagnani]

cc @kevinpschaaf, @notwaldorf, and @sorvell to make sure our rules are ok

[kevinpschaaf]

The new rules look 👍 to me.

I filed an issue on iron-doc-viewer to change from "Show Private" to "Show Protected".

[justinfagnani]

Fixed in #525