Skip to content
This repository was archived by the owner on Dec 23, 2021. It is now read-only.

Basic functionality for microbit Debugging and fixes #216

Merged
merged 28 commits into from
Feb 24, 2020

Conversation

andreamah
Copy link
Contributor

Description:

Improvements to CPX debugger and addition of microbit debugger.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Limitations:

Testing:

Checklist:

  • My code follows the style guidelines of this project
  • My code has been formatted with npm run format and passes the checks in npm run check
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules

@xnkevinnguyen xnkevinnguyen changed the title WIP: Revised Debugger Basic functionality for microbit Debugging and fixes Feb 21, 2020
Copy link
Contributor

@isadorasophia isadorasophia left a comment

Choose a reason for hiding this comment

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

Nice job everyone! 🐊

);
});
socket.on(DEBUGGER_MESSAGES.LISTENER.RECEIVED_STATE, () => {
this.isWaitingResponse = false;
Copy link
Contributor

@isadorasophia isadorasophia Feb 24, 2020

Choose a reason for hiding this comment

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

We can alternatively only assign this to false if this.currentCall.length != 0 -- then we wouldn't need to update this value inside currentCall() again.

Copy link
Contributor

Choose a reason for hiding this comment

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

good suggestion! Removed it from current call and setting it to true if this.currentcall.lenth>0. If it's 0, then i'm putting it to false since there's no more calls

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we don't need to assign it to true on the listener? Since we won't assign it to false elsewhere. Anyway, it's up to you if you think that's a good sanity check!

Copy link
Contributor

@isadorasophia isadorasophia left a comment

Choose a reason for hiding this comment

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

Just a heads up, try to keep only one change per PR (e.g. I noticed we have some stuff for telemetry as well). It's easier to roll back if there is any breaking changes if we keep one thing per insertion.
I think that's all right for now, just to keep that in mind for the next ones!

@xnkevinnguyen xnkevinnguyen merged commit 389a2b2 into dev Feb 24, 2020
@xnkevinnguyen xnkevinnguyen deleted the users/t-anmah/debugger branch February 28, 2020 21:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants