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

Open declaration extension point #37

Open
wants to merge 3 commits into
base: testing
Choose a base branch
from

Conversation

frerich
Copy link

@frerich frerich commented Apr 11, 2017

This extension point permits providing additional algorithms for
implementing the 'Open Declaration' functionality such that it works not
only for packages and subs.

The AbstractOpenDeclaration class has package-visibility and exposes a
bit more functionality than I'd like, so I decided to introduce anew
IOpenDeclarationHandler interface and expect extensions to implement
that. A ContributedOpenDeclarationHandler action wraps this interface
and forwards all calls.

Frerich Raabe added 2 commits April 7, 2017 09:21
This extension point permits providing additional algorithms for
implementing the 'Open Declaration' functionality such that it works not
only for packages and subs.

The AbstractOpenDeclaration class has package-visibility and exposes a
bit more functionality than I'd like, so I decided to introduce anew
IOpenDeclarationHandler interface and expect extensions to implement
that. A ContributedOpenDeclarationHandler action wraps this interface
and forwards all calls.
That way the errors are written to Eclipse' workspace log file and show up in
the Error Log View in Eclipse itself too.
@otrosien
Copy link
Contributor

@frerich very interesting PRs. Can you elaborate on the background, is there a ticket, feature request or specification you followed somewhere?

@frerich
Copy link
Author

frerich commented May 13, 2017

@otrosien Indeed, there were multiple feature requests motivating these PRs. Our product (which comes with an IDE for which EPIC serves as the Perl editor), features custom Perl APIs which can be used to include external files (see #38). In some cases, those external files define an 'object repository' establishing a set of 'object names' referencing controls in a GUI. The script looks much like this (ObjectName is another API offered by our product):

our $gui = {};

$gui->{item_views_mainwindow} = ObjectName->new({"type" => "MainWindow", "windowTitle" => "Item Views"});
$gui->{item_views_qlistview}  = ObjectName->new({"type" => "QListView", "window" => $gui->{item_views_mainwindow}});

To recognize terms like $gui->{item_views_mainwindow} as identifiers, it appeared to be necessary to extend the tokenizer (see #39) - in our extension, we maintain a list of seen identifiers following the format $gui->{...}. In order to be able to jump to these definitions by considering our list of identifiers, we the need to be able to hook into the 'Open Declaration' functionality, which is what this PR is about.

@otrosien
Copy link
Contributor

@jploski what do you think about the changes? I can't judge really because I'm not too deep into the code really.

@jploski
Copy link
Owner

jploski commented May 17, 2017

I really have mixed feelings about this - these contributed extensions seem quite specific and are not well documented (in fact, there are "document me" placeholders in their manifests).

Introducing a couple extension points (of which EPIC doesn't have any as of yet) in lieu of a well thought-out and carefully specified API for third parties could be a wrong move. And it's a long-term commitment to maintain, as we cannot change once published APIs frivolously.

I understand the purpose of these extensions in context of the third-party product, but maybe they are best kept as a product-specific patch rather than integrated in the public code base. Either that, or they would have to be generalized (e.g. EPIC should also use its own Open Declaration extension by providing a default implementation) and documented so that the contract which needs to be upheld is clear to both EPIC developers and third parties.

These imports where lost when the logging was improved in
OpenDeclarationAction
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.

3 participants