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

Use Named Listeners in DebugSession #8616

Merged
merged 1 commit into from
Oct 27, 2020
Merged

Use Named Listeners in DebugSession #8616

merged 1 commit into from
Oct 27, 2020

Conversation

colin-grant-work
Copy link
Contributor

@colin-grant-work colin-grant-work commented Oct 9, 2020 •
edited
Loading

Signed-off-by: Colin Grant colin.grant@ericsson.com

What it does

This PR addresses #8614 with respect to the DebugSession by extracting the larger anonymous functions and assigning them to member variables. At present, it would be necessary to override the whole constructor to change any individual listener, and that makes it more difficult for adopters to keep in synch with changes upstream.

Pros:

  • makes it much easier for adopters to customize the behavior of the class' response to these debug events.

Cons:

  • if any downstream projects have implemented their own functions with the names used for these handlers, it could break their implementations with conflicting typedefs or just unexpected behavior.

How to test

It should have no effect on application behavior.

Review checklist

Reminder for reviewers

@vince-fugnitto vince-fugnitto added debug issues that related to debug functionality extensibility issues to simplify ability to extend Theia labels Oct 9, 2020
kittaakos
kittaakos reviewed Oct 12, 2020
View reviewed changes
packages/debug/src/browser/debug-session.tsx Outdated
@@ -759,4 +741,27 @@ export class DebugSession implements CompositeTreeElement {
return this.threads;
}

protected handleContinued = ({ body: { allThreadsContinued, threadId } }: DebugProtocol.ContinuedEvent): void => {
Copy link
Contributor

Choose a reason for hiding this comment

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

From here:

only that good practice that doesn't hurt anything upstream

Let's make it return with Promise<void> so downstreams have greater flexibility on the code customization. Same for handleThread. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds fair to me - I'll make the change.

@colin-grant-work
use named listeners in DebugSession
e99cd03 
Signed-off-by: Colin Grant <colin.grant@ericsson.com>
@colin-grant-work colin-grant-work force-pushed the extensibility/debug-session-event-handlers branch from 97b34e9 to e99cd03 Compare October 12, 2020 14:15
akosyakov
akosyakov approved these changes Oct 13, 2020
View reviewed changes
Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

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

I have not tried, but code wise looks good.

vince-fugnitto
vince-fugnitto approved these changes Oct 13, 2020
View reviewed changes
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

The changes look good to me, I verified if there were any regressions when starting a debug session (ex: run mocha tests). The debug session actions successfully worked (ex: paused, started, stopped, step over, step into...).

@vince-fugnitto
Copy link
Member

@colin-grant-work would you like to merge, I believe you have the proper permissions now :)

@vince-fugnitto vince-fugnitto merged commit e07af50 into eclipse-theia:master Oct 27, 2020
@vince-fugnitto
Copy link
Member

@marcdumais-work I merged for Colin as I believe he did not have the proper permissions yet to do so despite now being a committer, is there some sync we need to perform on the repo rights?

@marcdumais-work
Copy link
Contributor

Hi,

@colin-grant-work @kenneth-marut-work @Anasshahidd21 I think you may all be in the same situation. I see none of you listed as members of the eclipse-theia GH org: https://github.com/orgs/eclipse-theia/people?query=

  1. please make sure you have not missed an email from GH, with an invite to become a member of the above org. There's a link you need to click to accept the invite. Check your spam folder.
  2. if not, there may be a problem with the synchronisation script, that automatically updates the GH orgs' and project's memberships to match the Eclipse Foundation's project committer's list.

Please confirm that 1 is done, and I'll open a bugzilla about 2.

@vince-fugnitto vince-fugnitto mentioned this pull request Nov 6, 2020
Closed
@colin-grant-work colin-grant-work deleted the extensibility/debug-session-event-handlers branch April 29, 2021 19:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vince-fugnitto vince-fugnitto vince-fugnitto approved these changes

@akosyakov akosyakov akosyakov approved these changes

@kittaakos kittaakos kittaakos approved these changes

Assignees
No one assigned
Labels
debug issues that related to debug functionality extensibility issues to simplify ability to extend Theia
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@colin-grant-work @vince-fugnitto @marcdumais-work @kittaakos @akosyakov