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.

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

fix: Use real name of the tag if it is unknwon #3513

Merged
merged 16 commits into from
Sep 11, 2020

Conversation

zxdxjtu
Copy link
Contributor

@zxdxjtu zxdxjtu commented Jul 30, 2020

When generating LSIF file using spoon, we use (CtClass)ctClass.getDocComment to fetch doc comment of the class to display hover result.
However, spoon regards user-defined tag (e.g. @email @create ) as '@unknown', it is really unfriendly for printing hover content. Usually the class hover would be:
Example class
@author zxdxjtu
@unknwon [email protected]
@unknown 2020-07-30 10:24

To solve this problem, I retain the real name of the comment tag and use real name of the rag instead of TagType.name when printing doc comment to avoid '@unknown' condition.

别象 added 3 commits July 30, 2020 09:59
Given the scenario that we need to show javadoc comment on LSIF hover, we need realname of the tag
instead of UNKNOWN tag
Retain real name when parsing javadoc comment, and pretty print the realname tag instead of using
UNKNOWN
Copy link
Collaborator

@monperrus monperrus left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your contribution. That would be super useful. To proceed:

  • See PR comments
  • Could you add a test case to specify the new feature?
  • See CI failures

Thanks again!

@zxdxjtu
Copy link
Contributor Author

zxdxjtu commented Aug 5, 2020

Forgive me for mistakes on code modification. Recently I don't have much time on fixing failed test cases, I will perfect it as soon as possible.

@zxdxjtu zxdxjtu requested a review from monperrus August 17, 2020 06:20
Copy link
Collaborator

@monperrus monperrus left a comment

Choose a reason for hiding this comment

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

Thanks, we're making good progress.

Now we need to add a couple of test cases, to specify this new functionality and cover all added lines.

Thanks again!

@@ -151,7 +151,7 @@ public void testGenerateCloneVisitor() {
CtClass<Object> expected = build(new File("./src/main/java/spoon/support/visitor/clone/CloneBuilder.java")).Class().get("spoon.support.visitor.clone.CloneBuilder");
assertThat(actual).isEqualTo(expected);

// cp ./target/generated/spoon/support/visitor/clone/CloneVisitor.java ./src/main/java/spoon/support/visitor/clone/CloneVisitor.java
Copy link
Collaborator

Choose a reason for hiding this comment

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

undo

别象 added 2 commits August 27, 2020 14:09
…own type match problem

tag name is UNKNOWN, but TagType.UNKNOWN.getName() is unknwon, should compare ignore case
@zxdxjtu zxdxjtu requested a review from monperrus August 27, 2020 07:05
@monperrus
Copy link
Collaborator

Hi @zxdxjtu Thanks for the ping. I don't see new changes in the diff since my previous review: #3513 (review)

Any idea?

@monperrus
Copy link
Collaborator

FYI, this stackoverflow question seems about this: https://stackoverflow.com/questions/63810364/getting-text-of-non-javadoc-tag-inside-doc-comment

@monperrus
Copy link
Collaborator

Hi @zxdxjtu we're almost there, one more change and failing tests to fox :)

Then we merge!

@zxdxjtu
Copy link
Contributor Author

zxdxjtu commented Sep 11, 2020

Hi, @monperrus , it seems that I have already fixed all travis issues before, but 'travis failed' appeared again when I merged master. Travis said multiple ' duplicate classes found', I am not so familiar with travis, could you help me find the way to fix travis issue?

@zxdxjtu zxdxjtu closed this Sep 11, 2020
@zxdxjtu zxdxjtu reopened this Sep 11, 2020
Copy link
Collaborator

@monperrus monperrus left a comment

Choose a reason for hiding this comment

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

Fixed CI by pushing to your branch. LGTM, will merge.

@monperrus monperrus self-assigned this Sep 11, 2020
@monperrus monperrus changed the title WIP: fix: Use real name of the tag if it is unknwon fix: Use real name of the tag if it is unknwon Sep 11, 2020
@monperrus monperrus merged commit 52d4df9 into INRIA:master Sep 11, 2020
@monperrus
Copy link
Collaborator

Thanks a lot for your contribution!

@monperrus monperrus mentioned this pull request Oct 14, 2020
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