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

Show only tests #1687

Merged
merged 1 commit into from
Oct 9, 2024
Merged

Show only tests #1687

merged 1 commit into from
Oct 9, 2024

Conversation

jannisCode
Copy link
Contributor

@jannisCode jannisCode commented Oct 1, 2024 •
edited
Loading

What it does

Add a new option to only show the test code when filtering the Call Hierarchy. The option was missing before since one could only either show all code or show only non-test code.

Before

image

After

image

Author checklist

fedejeanne
fedejeanne suggested changes Oct 2, 2024
View reviewed changes
Copy link
Contributor

@fedejeanne fedejeanne left a comment

Choose a reason for hiding this comment

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

Thank you for this contribution, @jannisCode!

I tested the changes and they produce the expected output ✔️

I only have some comments regarding formatting and unnecessary code. Once that is addressed, you can set this PR as "ready to review" so the community can also look at it.

...pulation/core extension/org/eclipse/jdt/internal/corext/callhierarchy/CallHierarchyCore.java Outdated Show resolved Hide resolved
...pulation/core extension/org/eclipse/jdt/internal/corext/callhierarchy/CallHierarchyCore.java Outdated Show resolved Hide resolved
...pulation/core extension/org/eclipse/jdt/internal/corext/callhierarchy/CallHierarchyCore.java Outdated Show resolved Hide resolved
...pulation/core extension/org/eclipse/jdt/internal/corext/callhierarchy/CallHierarchyCore.java Show resolved Hide resolved
...pulation/core extension/org/eclipse/jdt/internal/corext/callhierarchy/CallHierarchyCore.java Outdated Show resolved Hide resolved
...lipse.jdt.ui/core extension/org/eclipse/jdt/internal/corext/callhierarchy/CallHierarchy.java Outdated Show resolved Hide resolved
...lipse.jdt.ui/core extension/org/eclipse/jdt/internal/corext/callhierarchy/CallHierarchy.java Outdated Show resolved Hide resolved
...lipse.jdt.ui/core extension/org/eclipse/jdt/internal/corext/callhierarchy/CallHierarchy.java Outdated Show resolved Hide resolved
org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/callhierarchy/FiltersDialog.java Outdated Show resolved Hide resolved
org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/callhierarchy/FiltersDialog.java Outdated Show resolved Hide resolved
@jannisCode jannisCode force-pushed the Show_Only_Tests branch 3 times, most recently from e71a95e to f340d19 Compare October 2, 2024 14:11
@jannisCode jannisCode marked this pull request as ready for review October 4, 2024 06:33
@fedejeanne
Copy link
Contributor

@jannisCode I think the last amend changed the commit message. The commit message should follow these guidelines: https://github.com/eclipse-platform/.github/blob/main/CONTRIBUTING.md#commit_message_recommendations

jukzi
jukzi reviewed Oct 8, 2024
View reviewed changes
...lipse.jdt.ui/core extension/org/eclipse/jdt/internal/corext/callhierarchy/CallHierarchy.java Outdated
@@ -37,10 +37,14 @@
import org.eclipse.jdt.internal.ui.util.StringMatcher;

public class CallHierarchy {
private static final String PREF_USE_IMPLEMENTORS= "PREF_USE_IMPLEMENTORS"; //$NON-NLS-1$

private static final String PREF_SHOW_ALL_CODE = "PREF_SHOW_ALL_CODE"; //$NON-NLS-1$
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do these constants copy the values from CallHierarchyCore.java? Is it important that the values are the same? Then please reuse it (declare package x-friend).

https://github.com/eclipse-jdt/eclipse.jdt.ui/pull/1687/files#diff-9d62c5f4c6ca62f274aa1583e7e4f95060651e52e840b380013e205f1d67a21aR49?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In CallHierarchyCore I changed the three constants which I added to public and will create another PR for the three constants which have already been there when I started the issue and are also private.
In the other PR I will make them Public and change the code in CallHierarchy.

Copy link
Contributor

Choose a reason for hiding this comment

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

#1706

@jannisCode jannisCode force-pushed the Show_Only_Tests branch 3 times, most recently from 480f294 to 570bf79 Compare October 9, 2024 09:44
@jannisCode jannisCode mentioned this pull request Oct 9, 2024
Merged
3 tasks
fedejeanne
fedejeanne reviewed Oct 9, 2024
View reviewed changes
Copy link
Contributor

@fedejeanne fedejeanne left a comment

Choose a reason for hiding this comment

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

I tested it and it works. I only have 2 nit-picky suggestions on how to make the code more compact.

After that, it's good to go on my side.

org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/callhierarchy/FiltersDialog.java Outdated Show resolved Hide resolved
org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/callhierarchy/FiltersDialog.java Outdated Show resolved Hide resolved
org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/callhierarchy/FiltersDialog.java Show resolved Hide resolved
@jannisCode
Updated the filter feature in the call hierarchy
1879246 
changed the feature from only one selectable option to three radio
buttons which are more understandable and provide more functionality.
fedejeanne
fedejeanne approved these changes Oct 9, 2024
View reviewed changes
Copy link
Contributor

@fedejeanne fedejeanne left a comment

Choose a reason for hiding this comment

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

It works as expected and all comments (mine and @jukzi's) have been addressed.

@fedejeanne
Copy link
Contributor

Could you please merge this one too, @jukzi ? I don't have any committer rights on JDT :-)

@jukzi jukzi merged commit 3948ff8 into eclipse-jdt:master Oct 9, 2024
10 checks passed
@jannisCode jannisCode mentioned this pull request Oct 11, 2024
Closed
@fedejeanne
Copy link
Contributor

@jukzi I think this is "noteworthy", what do you say?

Should @jannisCode create an N&N entry?

@jukzi
Copy link
Contributor

jukzi commented Oct 15, 2024 via email

@jannisCode jannisCode mentioned this pull request Oct 15, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fedejeanne fedejeanne fedejeanne approved these changes

@jukzi jukzi jukzi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jannisCode @fedejeanne @jukzi