-
Couldn't load subscription status.
- Fork 121
Source repository link added in the Plug-in Selection Spy (2) #2023
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
Source repository link added in the Plug-in Selection Spy (2) #2023
Conversation
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.
Regarding #2000 (comment):
This PR has to be closed because of a commit name issue. Replaced by #2023
As explained in #2026 (comment), please try to avoid creating new PRs. For just adapting the change to suggestion that's not necessary.
Besides that this change looks better already. Thanks for the update and the delayed reply.
I just have a few minor follow-up remarks below.
ui/org.eclipse.pde.runtime/src/org/eclipse/pde/internal/runtime/spy/SpyFormToolkit.java
Outdated
Show resolved
Hide resolved
ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/util/ManifestUtils.java
Outdated
Show resolved
Hide resolved
ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/util/ManifestUtils.java
Outdated
Show resolved
Hide resolved
ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/util/ManifestUtils.java
Outdated
Show resolved
Hide resolved
ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/util/ManifestUtils.java
Outdated
Show resolved
Hide resolved
ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/util/ManifestUtils.java
Outdated
Show resolved
Hide resolved
|
@amelodev : merge commits are not allowed. Please always rebase your branch on top of master, do not merge! |
|
This pull request changes some projects for the first time in this development cycle. An additional commit containing all the necessary changes was pushed to the top of this PR's branch. To obtain these changes (for example if you want to push more changes) either fetch from your fork or apply the git patch. Git patchFurther information are available in Common Build Issues - Missing version increments. |
32e5fa0 to
30a62db
Compare
Sorry, i updated PR locally and removed the ugly merge commit :-) |
afd033e to
9c68ed3
Compare
d20ec77 to
1aa15ff
Compare
| String bundle = href.substring(BUNDLE_PROTOCOL_PREFIX.length()); | ||
| SpyIDEUtil.openBundleManifest(bundle); | ||
| fDialog.close(); | ||
| } else if (PATTERN.matcher(href).find()) { |
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.
If you use find() here having the regex to end with \\S+ is not really useful as it will just stop the match of the stubstring e.g. on the the first whiespace.
My proposal is to either use match(), which then requires the entire string to match the pattern (and in this case you can also remove the leading ^ from the pattern) or to continue with find(), but remove the trailing \\S+ from the pattern.
I personally would prefer the former.
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 have applied this and pushed the change to your PR branch. I also took the opportunity to make the variable name a bit more descriptive.
5bdaad6 to
43fda1f
Compare
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.
Thanks for this enhancment of the Spies.
I just have pushed an update with a few minor adjustments of the code, mainly style improvements and to fix the new warnings.
With that this should be ready for submission.
|
Thank you @amelodev and @HannesWell |
This request replace PR #2000, fixing an incorrect commit name.