* Changed Comment::Associator#associate API and way of associating comments #188

kubicle · 2015-03-07T16:41:15Z

This PR makes the "Comment Associator" behave differently to solve 2 issues and allow callers to make the difference between "heading" comments, which come before a piece of code, and "decorative" comments which come right after the code they are associated to.

Changes:

associator#associate now returns 2 hashes: @commentMap and @decorative_comment
@commentMap corresponds to the hash which the function used to return, but with a different key to fix the issue of "merged" comments for similar nodes (issue Comment.associate "merging" comments for identical nodes #184). The key is now the node's location instead of the node itself (the value is an array of comments, this did not change).
The new returned hash @decorative_comment uses the comment as key. The value is a boolean which is true if the comment is "decorative" (false for heading comments).

NB: Most of the time, a decorative comment is on the same line as the node it is associated to. The exception is for trailing comments in a file, which are after the last function of a file. The code in this PR will associate these comments correctly to the last function. This fixes issue #185.

Fixes #184
Fixes #185

Example of use:

  # Parse the source and comments
  ast, comments = Parser::CurrentRuby.new.parse_with_comments(srcFile)
  associator = Parser::Source::Comment::Associator.new(ast, comments)
  @commentMap, @decorative_comment = associator.associate
  ...

  # Returns comments of a node, ready for JavaScript format (a paragraph of heading comments, and a line of decorative comments)
  def getComments(node)
    comments = @commentMap[node.location] # used to be comments = @commentMap[node]
    parag = ""
    deco = ""
    comments.each do |c|
      txt = c.text[1..-1] # get rid of leading "#"
      txt = txt[1..-1] if txt[0]==" " # get rid of first space if any
      if @decorative_comment[c]
        deco << " // #{txt}"
      else
        parag << "// #{txt}\n"
      end
    end
    return parag, deco
  end

whitequark · 2015-03-08T02:15:01Z

No, unfortunately this is not a solution. The proper solution would:

Associate all comments with the corresponding nodes, with the "trailing" comments becoming also associated with the preceding node (like you did for the trailing comment in entire file).
Return an associative array, i.e. an array of pairs, where each pair consists of the node and the corresponding comment.

kubicle · 2015-03-09T18:11:10Z

1: I might have been unclear : this PR associates all comments.

2: And what would be the index of the array? Do you really mean an integer index 0..n? This would make the caller do useless efforts to retrieve comments associated with a node.
I commented on this earlier but did not get a reply.

whitequark · 2015-03-09T19:07:00Z

@kubicle

I've put emphasis on "all" but it probably should've been on "corresponding nodes". I strongly dislike the distinct maps for "normal" and "decorative" comments. It's not a decision that is parser's to make.
The index would not have any meaning. Since the node cannot be used as a hash key in our circumstances and that will not be changed (see the original PR for why), it's the only way.

kubicle · 2015-03-10T01:20:43Z

So I have been unclear, sorry for that. See my (imperfect) code sample, though, all comments are in the same map ("commentMap").
The other map ("decorativeComment") is just a convenience for the caller : since the comment associator in this PR is already comparing the position of the comments it makes sense to store the result so callers do not need to do it again.

whitequark · 2015-03-10T07:27:45Z

OK. My last comment on that still applies--there is no place for this distinction in parser's API. It is an implementation detail at most.

kubicle · 2015-03-11T12:24:46Z

Well, this is "your baby" so you decide, of course.

The current implementation (or the one you proposed) is less interesting for a "client" of parser like me, and I suppose it will be true for all the clients who have to do something with the comments parsed.

I still cannot exactly understand what bothers you with the idea of "decorative comments". Especially in a language like ruby where comments are on a single line.
Or is it because comments don't classify as important results of the parsing job in your eyes? For any translation or pretty printing task, I think they are just as important. No?

whitequark · 2015-03-11T12:29:22Z

"Decorative comments" are a semantic distinction that is not present at source level. It is not parser's job to opine on whether the comment is "normal" or "decorative" or whatever. Its job is merely to present all comments and the nodes they're closest to.

Besides, it's completely trivial to reproduce the distinction if wanted. node.loc.expression.begin <=> comment.loc.expression.begin.

kubicle · 2015-03-12T01:07:55Z

Ah, good, I think we are getting to a better understanding.
I completely agree with you about the job:
"present all comments and the nodes they're closest to."

This is on the "closest" point that I think the current implementation can be improved. We could call them by any other name than "decorative comments", which was probably a wrong choice from me, but I do believe these comments should be associated with the previous node. Systematically associate a comment with the next node makes it wrong. But maybe I am missing something.
When you say it is trivial, this is if (and only if) you are given both the node and the comment, then you can compare their location and see if the comment is on the same line than the node.
However, in the current implementation, for a code like below, the comment gets associated to the node of the next function (f2).

x = f1(y,z) # call f1 on y & z

def f2(n)
...

And, correct me if I am wrong, the algorithm to find that this comment was actually intended for the call of f1 is not trivial; you would have to backtrack the previous nodes - actually doing a similar tree visiting that comment associator has already done.
Do I make more sense to you now?

whitequark · 2015-03-12T01:11:26Z

Sure. In my view a proper implementation would associate a comment not on the beginning of line but at the end of line with the preceding node. Is this not what yours does?

olombart · 2015-03-12T15:48:32Z

Yes this is what it does. So we agree on this part, great!

Then the remaining issues are: (let me see if I got it correctly)

1- You would like the API to return an array of pairs [node, comment]; my PR is currently returning a hash [key=node-location, value=comment].
If we go your way, I can add an extra step to recreate the hash I need in my calling code (i.e. not in my PR but the code in my repo that uses Parser). I find this less good than returning my hash because I suppose that all callers will have to recreate this hash too. Reason is that they will visit the parsing tree finding each node in turn, and only at this moment (having the node) they will want to find the associated comment. With your array they would need a loop to find the right comment, while my hash would give it to them directly. Let me know if I am missing something here.

2- You are against returning the extra array of boolean, one per comment, with true for a comment associated with a preceding node, and false for one associated with a following node. I used to call the former "decorative" and I agree with you this is not right. I also agree this is easy for callers to compare the comment location with the node location and figure out this way if the comment is on the same line (or after the node, for the case of comments that are after the last statement of a function or the last function of a class, etc.).
I feel a bit (without 100% certitude) that other Parser users will have the same need as me (i.e. people for who just need to have the boolean "before-or-after-node" to they can decide how to use it without messing around with the location info)... But again, this is your call.

If the above is more or less correct, then would you be OK if I modify 1 & 2 according to what I described?
Namely, the API will simply return an array of pairs [node, comment]

kubicle · 2015-03-12T15:51:37Z

Ah, posted the above with my work account... This is still me ;)

whitequark · 2015-03-12T16:02:29Z

Oh! So you suggest using node.loc as the key instead of node. I agree that this is a better solution than an array.

kubicle · 2015-03-13T14:36:45Z

Very good! So just tell me what you want me to change in the PR and I will do it. Only removing @decorative_comment (and not return it to callers)? Anything else?

whitequark · 2015-03-13T14:41:57Z

Remove @decorative_comment
Don't change the interface; name the new function something like associate_locations and make sure the old function returns the data in the same format. The mapping would be straightforward.
Add tests!

kubicle · 2015-03-17T00:56:04Z

Hello!
Could you please take a look at the last commit above and tell me if this is what you meant?
About the tests:
I saw TestSourceCommentAssociator is breaking and could modify it to pass. In short, the "stray" comment would now be associated with previous node, and the intermediate comment would be with the node before it, I guess.
Thanks

whitequark · 2015-03-17T00:57:42Z

lib/parser/source/comment/associator.rb


        advance_through_directives if @skip_directives

        process(nil, @ast)

-        @mapping
+        return @mapping


Just @mapping, please

kubicle · 2015-03-17T00:59:41Z

Oh, and I can add a test for associate_locations of course!

whitequark · 2015-03-17T00:59:56Z

lib/parser/source/comment/associator.rb

-      def associate_and_advance_comment(node)
-        @mapping[node] << current_comment
+      def associate_and_advance_comment(prev_node, node)
+        if prev_node and node


whitequark · 2015-03-17T01:00:47Z

Yes, please

whitequark · 2015-03-17T01:00:52Z

lib/parser/source/comment/associator.rb

+      # node.location as a key to retrieve the comments for this node.
+      def associate_locations
+        @map_using_node = false
+        return associate


kubicle · 2015-03-20T01:32:04Z

Working on your changes...
It looks like I can simplify a bit and get rid of the "current_comment_decorates?" method.

whitequark · 2015-03-20T05:21:38Z

Sure

kubicle · 2015-03-24T00:06:57Z

@whitequark let me know how you like this one~

whitequark · 2015-03-24T00:19:05Z

lib/parser/source/comment/associator.rb

          end
-
-          prev_node, next_node = nil, upper_node
+          processTrailingComments(node)


process_trailing_comments

whitequark · 2015-03-24T00:22:18Z

Sans a few minor issues, this looks very good.

kubicle · 2015-03-26T16:10:27Z

Hey hey, what about now?

whitequark · 2015-03-26T16:54:07Z

lib/parser/source/comment/associator.rb

      #
-      def associate
+      def associate(map_using_locations = false)


This is not really what I meant... methods returning different types depending on an argument are horribly designed API.

Sigh. I'll just fix it myself

* Changed Comment::Associator#associate API and way of associating comments

whitequark · 2015-03-26T17:18:09Z

Please see the final API version on master.

Thanks for your work!

Changed API and way of associating comments + refact

3020c91

Restored the API of #associate, created #associate_locations

efa1e7b

whitequark reviewed Mar 17, 2015
View reviewed changes

Changed according to last review

98917fe

kubicle added 3 commits March 24, 2015 01:39

Improved logic to associate to children nodes

41aed28

Updated unit tests for comment associator

133be18

Restored path to "./helper" as "helper"

5164a8d

whitequark reviewed Mar 24, 2015
View reviewed changes

map_using_locations as param + style fixes + 1 test

895009b

whitequark reviewed Mar 26, 2015
View reviewed changes

whitequark added a commit that referenced this pull request Mar 26, 2015

Merge pull request #188 from kubicle/commentAssociator

ebbf32b

* Changed Comment::Associator#associate API and way of associating comments

whitequark merged commit ebbf32b into whitequark:master Mar 26, 2015

kubicle deleted the commentAssociator branch March 27, 2015 17:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

* Changed Comment::Associator#associate API and way of associating comments #188

* Changed Comment::Associator#associate API and way of associating comments #188

kubicle commented Mar 7, 2015

whitequark commented Mar 8, 2015

kubicle commented Mar 9, 2015

whitequark commented Mar 9, 2015

kubicle commented Mar 10, 2015

whitequark commented Mar 10, 2015

kubicle commented Mar 11, 2015

whitequark commented Mar 11, 2015

kubicle commented Mar 12, 2015

whitequark commented Mar 12, 2015

olombart commented Mar 12, 2015

kubicle commented Mar 12, 2015

whitequark commented Mar 12, 2015

kubicle commented Mar 13, 2015

whitequark commented Mar 13, 2015

kubicle commented Mar 17, 2015

whitequark Mar 17, 2015

kubicle commented Mar 17, 2015

whitequark Mar 17, 2015

whitequark commented Mar 17, 2015

whitequark Mar 17, 2015

kubicle commented Mar 20, 2015

whitequark commented Mar 20, 2015

kubicle commented Mar 24, 2015

whitequark Mar 24, 2015

whitequark commented Mar 24, 2015

kubicle commented Mar 26, 2015

whitequark Mar 26, 2015

whitequark commented Mar 26, 2015

* Changed Comment::Associator#associate API and way of associating comments #188

* Changed Comment::Associator#associate API and way of associating comments #188

Conversation

kubicle commented Mar 7, 2015

whitequark commented Mar 8, 2015

kubicle commented Mar 9, 2015

whitequark commented Mar 9, 2015

kubicle commented Mar 10, 2015

whitequark commented Mar 10, 2015

kubicle commented Mar 11, 2015

whitequark commented Mar 11, 2015

kubicle commented Mar 12, 2015

whitequark commented Mar 12, 2015

olombart commented Mar 12, 2015

kubicle commented Mar 12, 2015

whitequark commented Mar 12, 2015

kubicle commented Mar 13, 2015

whitequark commented Mar 13, 2015

kubicle commented Mar 17, 2015

whitequark Mar 17, 2015

Choose a reason for hiding this comment

kubicle commented Mar 17, 2015

whitequark Mar 17, 2015

Choose a reason for hiding this comment

whitequark commented Mar 17, 2015

whitequark Mar 17, 2015

Choose a reason for hiding this comment

kubicle commented Mar 20, 2015

whitequark commented Mar 20, 2015

kubicle commented Mar 24, 2015

whitequark Mar 24, 2015

Choose a reason for hiding this comment

whitequark commented Mar 24, 2015

kubicle commented Mar 26, 2015

whitequark Mar 26, 2015

Choose a reason for hiding this comment

whitequark commented Mar 26, 2015