Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Don't remove docs in block yielding of macro #3778

Merged
merged 3 commits into from
Dec 26, 2016
Merged

Don't remove docs in block yielding of macro #3778

merged 3 commits into from
Dec 26, 2016
+29 −20

Conversation

makenowjust
Copy link
Contributor

@makenowjust makenowjust commented Dec 25, 2016 •
edited
Loading

For example, we have a below code:

record Foo,
  foo : String do
  # this is cool
  def super_foo
    "Super #{foo}!"
  end
end

then run crystal doc, we get such a doc:

2016-12-26 1 01 50

There are no Foo#super_foo doc comment, it is just wrong. I fixed it by adding emit_doc optional arg to ASTNode#to_s and using it in macro interpreter.

@asterite
Copy link
Member

Good catch!

The emit_doc optional argument looks good, but the with_doc argument is not necessary: the doc property should always be cloned (copied) from the original node. This will simplify code a lot, and doesn't involve any performance or memory penalty.

@makenowjust
Copy link
Contributor Author

@asterite Thanks! I'll fix it.

@asterite
Copy link
Member

@makenowjust Great, thank you!! :-)

Now that I think about it, the comments are probably missing too when you declare a type inside record, or well you use record inside record with comment:

record Foo do
  # Bar comment
  record Bar

  # Baz comment
  class Baz
  end
end

This is because ClassDef, ModuleDef, EnumDef and Call also have a doc property, so this logic should probably be applied to those too. I can merge this PR and we (either me, you or somebody else) can do it later, or you can do it all in this PR? What do you prefer?

@makenowjust
Copy link
Contributor Author

@asterite I think it is no problem. Because doc property is copied by ASTNode#clone, not Def#clone_without_location, so we can copy doc every AST nodes.

@asterite asterite added kind:bug A bug in the code. Does not apply to documentation, specs, etc. kind:docs labels Dec 26, 2016
@asterite asterite added this to the 0.20.4 milestone Dec 26, 2016
@asterite
Copy link
Member

@makenowjust ありがとう！

@asterite asterite merged commit 225ff0d into crystal-lang:master Dec 26, 2016
@makenowjust makenowjust deleted the feature/crystal/to-s-emit-doc branch December 26, 2016 14:30
@makenowjust
Copy link
Contributor Author

@asterite Muchas gracias

makenowjust added a commit to makenowjust/crystal that referenced this pull request Dec 27, 2016
@makenowjust
Follow crystal-lang#3778 changes, support to expand doc in macro
5abf7a4
makenowjust added a commit to makenowjust/crystal that referenced this pull request Dec 27, 2016
@makenowjust
Follow crystal-lang#3778 changes, support to expand doc in macro
9dfcf86
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. kind:docs
Projects
None yet
Milestone
0.20.4
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@makenowjust @asterite