-
Notifications
You must be signed in to change notification settings - Fork 58
Conversation
@50Wliu @noseglid please review. |
'0': | ||
'name': 'punctuation.definition.comment.java' | ||
'end': '\\*/' | ||
'name': 'comment.block.java' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly have contentName: meta.comment.body.java
for consistency with class bodies, method bodies, try-catch-finally bodies etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I should replace 'name': 'comment.block.java'
with 'contentName': 'meta.comment.body.java'
in the whole comments-inline
, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean only for line 470 and line 510.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the name is correct (the tags /**
and */
will get name
). If you add contentName
then the content will get that name - which is the body of the comment. What do you think, @50Wliu ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is a Javadoc comment, would changing name
to be comment.block.javadoc.java
suffice? Do you think there would still be a benefit to having a separate contentName?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing to comment.block.javadoc.java
would work well I think
Javadoc comments should only be in the class (enum,interface etc) body, right? As it is now it also tags javadoc such as: class Test {
public static void main(String[] args) {
/**
* @throws somethingElse
*/
}
} Which may not be what we want. |
'name': 'entity.name.type.class.java' | ||
'3': | ||
'name': 'variable.parameter.java' | ||
'match': '\\{(@link)\\s+(\\S+)?#([^\\{\\}]+)\\}' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After #something
there can be a label
. This will be tagged with the variable.parameter.java
which is probably not correct. Adding that to the specs would be nice too
@noseglid Yeah, you are right about javadoc inside method, this "feature" seems to also exist in |
Well, I will think about how to possibly fix |
I think I would create a Great job on the PR btw, looks great overall, and very nice to get the Javadoc tagged properly! |
@@ -462,6 +462,46 @@ | |||
'comments-inline': | |||
'patterns': [ | |||
{ | |||
'begin': '^\\s*/\\*\\*' | |||
'captures': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make this a separate beginCaptures
and endCaptures
.
Can you give an example of an embedded code snippet? I might have some ideas for how to highlight that. Also left some minor style comments :). |
Thank you @noseglid @50Wliu, I will address comments later today. It seems like my first approach was suboptimal, I should have created a separate pattern for it. |
@50Wliu It is usually done using /**
* Usage:
* {@code
* int s = getSomething();
* boolean a = s > 1;
* }
*/
public static int getSomething() {
return -1;
} When I was trying it, I could not make it to treat I guess, |
Hmm yeah, that'll be tricky. When I have time I'll pull down this branch and see if I can figure out something that works. |
@noseglid @50Wliu I addressed your comments, added I am a little bit confused with:
Should I keep it this way, or should I change it to:
similar to class and method, or any other way? Have not tried multi-line |
I think you should remove Other than that this looks nice, I think. |
Ok, will do, thanks. |
@50Wliu @noseglid Rebased PR. Could you review it again, please? |
LGTM 👍 |
@50Wliu can you have a look at the PR? |
Been busy but I'll probably have some free time over the weekend or next week. |
'name': 'entity.name.type.class.java' | ||
} | ||
{ | ||
'match': '\\{(@link)\\s+(\\S+)?#([\\w$]+\\s*\\([^\\(\\)]*\\)).*\\}' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very small nit: the {
and }
don't need to be escaped.
@50Wliu I addressed your comments, would you mind taking a look again? |
Thanks! |
@50Wliu @noseglid Thank you very much for reviewing and merging pull request! |
This PR adds javadoc comments to
language-java
:@author
,@deprecated
,@exception
,@return
,@see
,@serial
,@since
,@throws
,@version
,@param
, and@link
, mentioned in issue #54. I tried adding highlighting for embedded code snippet in javadoc comments, but could not make it work.Used language-scala package mainly for guidance and implementation, since Atom packages are new to me.
Here is what it looks like in Atom:
Testing
Updated existing
tokenizes enum
test, added unit-tests.