-
Notifications
You must be signed in to change notification settings - Fork 61
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
Add jdwp support #61
Add jdwp support #61
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.
Great start !
I have a suggestion to organize code a bit differently. I would:
- move Type, Field, Struct, Command and Command set to
projects/jdwp/defs/schema.py
- move all remaining enums to
projects/jdwp/defs/constants.py
- move definitions of command sets to
projects/jdwp/defs/commandsets/...
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.
Not all commands are correctly described. In particular, some of them should have array fields and this PR does not add support for Arrays. Let's extract a part of this PR that can be landed sooner and will unblock @wekesa360.
Could you please do the following:
- address feedback in
schema.py
andconstants/*.py
- move all definitions of constants to
defs/constants.py
- for now remove
defs/commands/thread_reference.py
anddefs/command_sets/thread_reference.py
- all commands from
defs/commands/reference_type.py
but theSignature
command
- move definition of
Signature
fromdefs/commands/reference_type.py
todefs/command_sets/reference_type.py
- make
ReferenceType
command set incomplete, consisting of only one command (Signature
) for now - rebase on top of main and make sure code builds with buck and checks with pyre.
Could you also start files with this copyright ? |
Almost perfect! Pyre does not type check new files because they are not part of any buck target. Could you add
This will make all the new files visible to buck and pyre. |
projects/jdwp/BUCK
Outdated
@@ -8,7 +8,7 @@ python_binary( | |||
|
|||
|
|||
python_library( | |||
name = "lib", | |||
srcs = [], | |||
name = "defs", |
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.
This is not projects/jdwp/defs/BUCK
, but that is fine. In this case please keep the original name ("lib") and we should be good to go.
This PR tackles issue #56.
It creates a module for the JDWP commands and errors over here