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

Support Show as Binary #429

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

mylog00
Copy link

@mylog00 mylog00 commented Jul 24, 2022

Hi there!
This is PR for allow represent variables in binary forma.
I think it may be helpful for vscode-java-debug issue #1078
I've added new protocol parameter 'formatType' should contains variable format such as [BIN, OCT, HEX, DEC].
I've remain "showHex" for backward compatibility and can be delete later when PR to vscode-java-debug will be merged.
If you are not interested in this changes fill free to close this PR :)

@ghost
Copy link

ghost commented Jul 24, 2022

CLA assistant check
All CLA requirements met.

Comment on lines 27 to 28
@Deprecated
public boolean hex;
Copy link
Contributor

Choose a reason for hiding this comment

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

hex is part of the specification: https://microsoft.github.io/debug-adapter-protocol/specification#Types_ValueFormat, what's the rationale for deprecating this?

Copy link
Author

Choose a reason for hiding this comment

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

Because of new functionality we can format values to different formats now, beside hex. And I added new property ValueFormat#type instead of ValueFormat#hex to support that possibility. I assumed we can delete ValueFormat#hex in future with no harm because of no need and marked hex as deprecated. Just tell me if I did something wrong and I'll fix it. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

@mylog00 The data types under the folder com/microsoft/java/debug/core/protocol are generated from the DAP spec, we can't just change it on debugger side.

It's OK to extend the debug settings to support more format types, but remember to support the old valueFormat.hex as well.

Copy link
Author

Choose a reason for hiding this comment

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

I've removed @Deprecated annotation and added doc to new property ValueFormat#type. Support of the old valueFormat.hex already was in my code for backward compatibility.
Should I create PR to DAP or do something else?

Copy link
Contributor

Choose a reason for hiding this comment

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

When contributing to the DAP, you need to open an issue to discuss the proposal first. Only if the proposal is accepted, then open a PR.

A quick search and found a duplicated issue for more formatting options. microsoft/debug-adapter-protocol#197

Copy link
Author

Choose a reason for hiding this comment

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

Got it. Thanks)

Copy link
Author

Choose a reason for hiding this comment

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

Hi! If there is no other issue could you approve this PR?)

@mylog00 mylog00 requested review from mfussenegger and testforstephen and removed request for mfussenegger and testforstephen August 20, 2022 20:20
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