Skip to content

Membership inference improvements #558

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Oct 5, 2016
Merged

Membership inference improvements #558

merged 6 commits into from
Oct 5, 2016

Conversation

chkt
Copy link
Contributor

@chkt chkt commented Oct 5, 2016

Added normalization of @memberof Foo# and @memberof Foo.prototype
Added membership inference for this.foo inside constructor functions and class members

var memberof = comment.memberof;
var index = memberof.lastIndexOf('.');

if (index !== -1 && memberof.slice(index + 1) === 'prototype') {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could these index-based checks be expressed more simply with regular expressions? I believe this one would be memberof.match(/\.prototype$/)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know how important performance is to you. It surely would result in more compact code. But the next line would read comment.memberof = memberof.slice(0, -10); which looks a little bit to much on the magical side for my taste.

I could also use a capturing regexp: memberof.match(/^(.*).prototype$/) and use match[1] like this:

var match = memberof.match(/^(.*).prototype$/);

if (match !== null) {
  comment.memberof = match[1];

I'm not sure if that's an improvement in the end. But if this works for you:

  if (memberof.match(/.prototype$/) !== null) {
    comment.memberof = memberof.slice(0, -10);
    comment.scope = 'instance';

    return comment;
  }

I am totally on board.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably the most compact way to express this would be:

  var isPrototype = /.prototype$/;
  if (memberof.match(isPrototype)) {
    comment.memberof = memberof.replace(isPrototype, '');
    comment.scope = 'instance';
    return comment;
  }

Performance certainly matters, but with tiny strings in this place I don't think even a wasteful implementation could take more than 1% of total time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


var last = memberof.length - 1;

if (memberof[last] === '#') {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and this check would be memberof.match(/#$/)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and this is also done.

@tmcw
Copy link
Member

tmcw commented Oct 5, 2016

👍 Thanks Christoph!

@tmcw tmcw merged commit 473a7a2 into documentationjs:master Oct 5, 2016
@chkt
Copy link
Contributor Author

chkt commented Oct 5, 2016

:octocat:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants