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

C sharp samsung #36

Closed
wants to merge 5 commits into from
Closed

C sharp samsung #36

wants to merge 5 commits into from

Conversation

jpfeiffer16
Copy link
Contributor

@puremourning, these are my changes for #35.

I made three changes here:

  1. When doing requests to lookup variable requests, if the individual variable response elements do not have an 'expensive' key just assume they are not. Currently it is throwing a key error there.
  2. Fixed what I think was probably a typo in the event handler for OnEvent_capabilitiesthat was making so the capabilities never got set and were not respected afterwards. In this case it was supportsConfigurationDoneRequest.
  3. I added back in the mono debugger adapter. I know I mentioned this earlier and that might have led to some confusion. I don't think you need to remove this as this is a openly licensed debug adapter (I believe. I haven't done extensive research though so correct me if I'm wrong.) I checked this back in under the impression that I misspoke and that you removed it thinking it was the proprietary microsoft dotnet runtime debugger. If this is not the case and you had another reason, I will gladly revert the commit. Sorry for the confusion here.

@puremourning
Copy link
Owner

This change is Reviewable

@jpfeiffer16
Copy link
Contributor Author

I'm trying to run the tests locally to check the failures but I'm getting an error on the which command in the tests script ./run_tests. Illegal option -s

@nosami
Copy link

nosami commented Jun 22, 2019

The Mono debugger adapter is OSS

@puremourning
Copy link
Owner

Thanks for the PR!

FWIW, There’s a docker image that you can use to run the tests locally in the same image that azure runs (on Linux).

Looks like there were CI failures.

@puremourning
Copy link
Owner

Regarding the c# debug adapter, when I looked before I found there were licensing restrictions for one and I couldn’t make another work. I’m not precious about any particular adapter. I don’t actually use c# so provided there’s no license conflict im happy to go with any that works best. We use omnisharp in ycmd so that seems fine.

Copy link
Owner

@puremourning puremourning left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 3 files reviewed, 2 unresolved discussions (waiting on @jpfeiffer16)


install_gadget.py, line 146 at r1 (raw file):

  },
  'vscode-mono-debug': {
    'language': 'csharp',

Does this mean —enable-csharp installs both netcoredbg and mono debug?


python3/vimspector/variables.py, line 116 at r1 (raw file):

          scope[ '_expanded' ] = old_scopes[ i ].get( '_expanded', False )
          scope[ '_old_variables' ] = old_scopes[ i ].get( '_variables', [] )
        elif not 'expensive' in scope or not scope[ 'expensive' ]:

Prefer not scope.get( ‘expensive’ )

Copy link
Owner

@puremourning puremourning left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 3 files reviewed, 2 unresolved discussions (waiting on @jpfeiffer16)


install_gadget.py, line 146 at r1 (raw file):

Previously, puremourning (Ben Jackson) wrote…

Does this mean —enable-csharp installs both netcoredbg and mono debug?

Actually, no it means that install_gadget.py breaks :/

I think we should just remove this and go with the server that (i.e. the one you tested with). I'm not super keen to support 2 servers for the same language. Like ycmd, i'd rather pick the "best" one that we can, and use that.


python3/vimspector/variables.py, line 116 at r1 (raw file):

Previously, puremourning (Ben Jackson) wrote…

Prefer not scope.get( ‘expensive’ )

This change would also fix CI

Copy link
Owner

@puremourning puremourning left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 3 files reviewed, 2 unresolved discussions (waiting on @jpfeiffer16)


python3/vimspector/variables.py, line 116 at r1 (raw file):

Previously, puremourning (Ben Jackson) wrote…

This change would also fix CI

Checking the spec. this element is mandatory, so the server should be fixed.

Copy link
Contributor Author

@jpfeiffer16 jpfeiffer16 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @puremourning)


install_gadget.py, line 146 at r1 (raw file):

Previously, puremourning (Ben Jackson) wrote…

Actually, no it means that install_gadget.py breaks :/

I think we should just remove this and go with the server that (i.e. the one you tested with). I'm not super keen to support 2 servers for the same language. Like ycmd, i'd rather pick the "best" one that we can, and use that.

Ok, my bad on that. Didn't realize they had the same key. Removed it from the pr. One thing to note is that though these are for the same language the runtime itself is different. You couldn't to my knowledge debug an app using the mono runtime (OmniSharp-Roslyn on Linux or macOS for example) with this new netcoredbg adapter.


python3/vimspector/variables.py, line 116 at r1 (raw file):

Previously, puremourning (Ben Jackson) wrote…

Checking the spec. this element is mandatory, so the server should be fixed.

Changed to .get. I'll open an issue on the repo. I can change this back if that's resolved and you like to not worry about that case.

Copy link
Owner

@puremourning puremourning left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @jpfeiffer16 and @puremourning)


python3/vimspector/variables.py, line 116 at r1 (raw file):

Previously, jpfeiffer16 (Joe Pfeiffer) wrote…

Changed to .get. I'll open an issue on the repo. I can change this back if that's resolved and you like to not worry about that case.

we'll keep the workaround... this wouldn't be the first server that doesn't obey the protocol. It's a de facto protocol. If it works with vscode then server developers just don't care.

Copy link
Owner

@puremourning puremourning left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @jpfeiffer16)


install_gadget.py, line 146 at r1 (raw file):

Previously, jpfeiffer16 (Joe Pfeiffer) wrote…

Ok, my bad on that. Didn't realize they had the same key. Removed it from the pr. One thing to note is that though these are for the same language the runtime itself is different. You couldn't to my knowledge debug an app using the mono runtime (OmniSharp-Roslyn on Linux or macOS for example) with this new netcoredbg adapter.

oh I seeeeeeeeee.

I'm just showing my bad understanding of the c-sharp landscape.

If I understand, then

  • netcoredbg will debug dotnet core e.g. from dotnet new
  • mono debugger will debug any mono-based runtime app

Right. So there's probably scope for both to exist in a future where someone makes a mono debugger that we can exploit :)

@puremourning puremourning mentioned this pull request Jun 23, 2019
@mergify mergify bot closed this in #37 Jun 23, 2019
@puremourning
Copy link
Owner

Thanks for the PR! I used your commits and added a couple of my own (docs, minimal test so I can check it)

@jpfeiffer16
Copy link
Contributor Author

No problem! Thanks for using it.

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.

None yet

3 participants