add sublime_debug

[daveleroy]

Adds a graphical debugger for sublime text using the debug adapter protocol

[packagecontrol-bot]

Automated testing result: ERROR

Repo link: sublime_debug
Results help

Packages added:
  - sublime_debug

Processing package "sublime_debug"
  - ERROR: No valid semver tags found at https://github.com/daveleroy/sublime_debug/tags for the package "sublime_debug".

[FichteFoll]

I'm afraid you'll have to think of a different name because we don't allow new packages with "Sublime" in their name, or at the very least not at the beginning.

Other than that, I highly recommend using relative imports within your plugins (from .modules import ui) and the __package__ module-level global to determine the package's name dynamically for resource loading (__package__.split('.', 1)[0] will always yield the current package's name).

I see you are reading css files from the file system directly. If you switch that to use sublime.load_resource and specify a resource path, you shouldn't need .no-sublime-package anymore. Or at least I haven't found another reason indicating that you'd need it. (https://github.com/daveleroy/sublime_debug/blob/master/modules/ui/__init__.py#L38)

You're also using mixed whitespace in a couple files, but that's just a heads-up.

For logging, you may also be interested in using the logging module from the stdlib. Here's an example of how to use it in a plugin.

Other than that, I only took brief looks into various source files and this generally seems pretty good. Especially the usage of asyncio is probably the first I've reviewed so far.

Edit: Can't seem to get the review bot endpoint to work atm.

[FichteFoll]

Ran the review locally. Only warnings, so nothing severe, but something you should look at regardless

Report for sublime_debug

Repository checks

No failures

No warnings

Package checks

No failures

Reporting 4 warnings:

• Command class 'SublimeDebugShowLine' does not end with 'Command'
  File: modules/debugger/commands/debugger.py
  Line: 137, Column: 1
• Found multiple command prefixes: Invisible, Run, Sublime, Window. Consider using one single prefix so as to not clutter the command namespace.
• '.no-sublime-package' is defined. Please verify that it is really necessary
• It looks like you're using platform-dependent code. Make sure you thought about the platform key in your pull request. Also consider replacing the platform module with sublime.platform() and sublime.arch().
  File: modules/libs/asyncio/test_support.py
  Line: 11, Column: 1

For more details on the report messages (for example how to resolve them), go to:
https://github.com/packagecontrol/py_package_reviewer/wiki

[daveleroy]

Awesome thanks for the feedback. I'll try to make these changes this weekend.

[packagecontrol-bot]

Automated testing result: WARNING

Repo link: debugger
Results help

Packages added:
  - debugger

Processing package "debugger"
  - WARNING: '.no-sublime-package' is defined. Please verify that it is *really* necessary

[Thom1729]

Where is the bundled asyncio from? I didn't think that there was a 3.3 version.

Can you use relative imports so you don't have to modify sys.path?

[daveleroy]

@Thom1729 The asyncio is from https://github.com/python/asyncio/tree/master with some removed features to get it working on windows.

I can use relative imports if really needed. sys.path is only modified if its not installed from package control (only if the package name is not "debugger")

[daveleroy]

Switched over to relative imports

[FichteFoll]

sys.path wasn't actually modified and instead sys.modules, but using relative imports is always preferrable.

It would be nice if you read your resources using sublime.load_resource and sublime.load_binary_resource instead of the builtin open as that would allow getting rid of the .no-sublime-package file and make updates easier while giving users the ability to create overrides. However, if you don't want to deal with this for now and prefer to tackle it later, that would be fine with me.

You may also be interested in the sublime_lib dependency that has various utility classes and functions for plugins (disclaimer: I am part of the project).

[Thom1729]

The asyncio is from https://github.com/python/asyncio/tree/master with some removed features to get it working on windows.

You should probably include the original asyncio license along with that module code, then.

[daveleroy]

The asyncio is from https://github.com/python/asyncio/tree/master with some removed features to get it working on windows.

You should probably include the original asyncio license along with that module code, then.

Thanks must have got lost along the way

[daveleroy]

sys.path wasn't actually modified and instead sys.modules, but using relative imports is always preferrable.

It would be nice if you read your resources using sublime.load_resource and sublime.load_binary_resource instead of the builtin open as that would allow getting rid of the .no-sublime-package file and make updates easier while giving users the ability to create overrides. However, if you don't want to deal with this for now and prefer to tackle it later, that would be fine with me.

You may also be interested in the sublime_lib dependency that has various utility classes and functions for plugins (disclaimer: I am part of the project).

Sorry sys.modules is what I meant. Its fully using relative paths and it no longer modifies sys.modules. I think there are few other changes required to remove .no-sublime-package currently when installing an adapter it downloads them from their corresponding vscode package and stores them in debugger/data/debbug_adapters. It also is currently storing persisted breakpoints/selected configuration in debugger/data. Is there better place to persist this data?

[Thom1729]

currently when installing an adapter it downloads them from their corresponding vscode package and stores them in debugger/data/debbug_adapters.

It should be safe to store this in Cache/debugger (under sublime.cache_path()).

It also is currently storing persisted breakpoints/selected configuration in debugger/data.

Could this be stored in preferences? If not, the Cache directory might work for this as well.

Also, a minor point, but is there a reason that the package name is lowercase? Usually package names are capitalized and dependency names are lowercase, though I don't think it's a rule.

[packagecontrol-bot]

Automated testing result: WARNING

Repo link: debugger
Results help

Packages added:
  - debugger

Processing package "debugger"
  - WARNING: '.no-sublime-package' is defined. Please verify that it is *really* necessary

[packagecontrol-bot]

Automated testing result: WARNING

Repo link: Debugger
Results help

Packages added:
  - Debugger

Processing package "Debugger"
  - WARNING: '.no-sublime-package' is defined. Please verify that it is *really* necessary

[daveleroy]

Could this be stored in preferences? If not, the Cache directory might work for this as well.

Not nicely. It's all the metadata for each breakpoint they have in their project. I don't think people want that polluting their preference/project files.

[daveleroy]

Also, a minor point, but is there a reason that the package name is lowercase? Usually package names are capitalized and dependency names are lowercase, though I don't think it's a rule.

No reason. Changed to capitalized

[FichteFoll]

You'll need this document as well, since COPYING only holds the license terms, not the license holders (although they didn't really do this properly either).

Anyway, I believe we can merge this now. I'll create an issue for phasing out .no-sublime-package on the repo, so it's not forgotten.

I'm excited to see how well it integrates into the "IDE ecosystem" alongside LSP and when people start to catch on to it.