Go directly to thrift declaration #139
Conversation
azdrojowa123
force-pushed
the
goTo-declaration
branch
from
ebaf7e0
to
4ed791c
Compare
azdrojowa123
force-pushed
the
goTo-declaration
branch
from
4ed791c
to
ac2aca8
Compare
| .map(name -> ThriftDeclarationIndex.findDeclaration(getElementName(name), getPackageName(name), project, GlobalSearchScope.allScope(project))) | ||
| .flatMap(Collection::stream) | ||
| .collect(Collectors.toList()); | ||
| } catch (NoClassDefFoundError ignored) {} |
There was a problem hiding this comment.
I am curious when does this exception happen
There was a problem hiding this comment.
Now I am curious too - I didn't think about this try.. catch block by myself (this try..catch block come from GoToThriftDefinition#getThriftDeclarations) Now I don't see any place where it could be thrown
| } | ||
|
|
||
| @Override | ||
| public @Nullable @Nls(capitalization = Nls.Capitalization.Title) String getActionText(@NotNull DataContext context) { |
There was a problem hiding this comment.
Is it required to override this method if it just calls super?
| private String getPackageName(String qualifiedName) { | ||
| try { | ||
| return qualifiedName.substring(0, qualifiedName.lastIndexOf(".")); | ||
| } catch (IndexOutOfBoundsException ex){ |
There was a problem hiding this comment.
Let's avoid exception driven development. Just check if index of dot is >= 0 and return null if not.
There was a problem hiding this comment.
Same for getElementName
|
|
||
| @Override | ||
| public PsiElement @Nullable [] getGotoDeclarationTargets(@Nullable PsiElement sourceElement, int offset, Editor editor) { | ||
| assert sourceElement != null; |
There was a problem hiding this comment.
Is it good to throw AssertionError in such case? sourceElement is nullable. You could just return null.
| List<String> fullyQualifiedNames = Collections.emptyList(); | ||
|
|
||
| if (psiReference instanceof PsiJavaCodeReferenceElement) { | ||
| fullyQualifiedNames = Arrays.stream(((PsiJavaCodeReferenceElement) psiReference).multiResolve(false)) |
There was a problem hiding this comment.
can you add /*paramName = */ for the boolean parameter, or extract this false to a named variable, so that it is easier to understand what it means?
| assert sourceElement != null; | ||
|
|
||
| PsiFile containingFile = sourceElement.getContainingFile(); | ||
| PsiReference psiReference = containingFile.findReferenceAt(sourceElement.getTextRange().getStartOffset()); |
There was a problem hiding this comment.
See if getContainingFile can be null, if so, add a null check.
| for (PsiElement child : psiFile.getChildren()) { | ||
| if (child instanceof ThriftTopLevelDeclaration && name.equals(((ThriftDeclaration)child).getName())) { | ||
| result.add((ThriftDeclaration)child); | ||
| file -> { |
There was a problem hiding this comment.
You could align indents so this method looks similar to the one above
No description provided.