Skip to content
This repository was archived by the owner on Feb 28, 2022. It is now read-only.

Updated to work with Sublime Text 4#76

Open
munkybutt wants to merge 49 commits intojgirardet:masterfrom
munkybutt:master
Open

Updated to work with Sublime Text 4#76
munkybutt wants to merge 49 commits intojgirardet:masterfrom
munkybutt:master

Conversation

@munkybutt
Copy link

I have made a number of changes to make this plugin compatible with Sublime Text 4:

  • Blackd server now starts up in a cleanly in a background thread without blocking the ui
  • Removed the need for the watcher as ST4 now supports on_exit event
  • Added more verbose and cleaner to read logging
  • Formatted code with black

munkybutt added 6 commits June 2, 2021 16:15
Running blackd sublime command using sublime async api call locks up the ui - moved the function to spawn blackd server into a utils function and call it separately from sublime commands
Updated init of blackd to be correctly async - running a command async seems to not work as expected.
Reduced time to test if port number is free and if black is running.
Unified logger logic and added ability to log to file.
Removed checker and related logic as ST4 as on_exit event.
Added better error handling for when black or blackd cannot be found with the given command in settings
@jgirardet
Copy link
Owner

it's a big one, i'll try to review it soon. thanks

@munkybutt
Copy link
Author

Amazing!
Sorry for the size. There were a number of interconnected things to modify to resolve the blackd server startup.
If there is any way I can help make it more digestible, let me know.
I could try breaking it into smaller PRs but that would make it non-functional.
The other thing is this will break compatibility with ST3.

@haferburg
Copy link

changes to make this plugin compatible with Sublime Text 4

I've been using sublack with ST4 for a while now, and I haven't had problems. As far as I can tell, sublack is already compatible with ST4. Maybe you could reword that? What made you work on this?

Formatted code with black

I don't know for sure, but the thought that the code of sublack wasn't already formatted with black seems a little absurd to me. Could it be that you use a different black_line_length setting?

Your commits mix formatting changes with code changes, which makes it harder on the reviewer. There's multiple changes packed into one commit. You even have a typo in a commit message: "formatted all files with back :)".

I'm not a maintainer of this repo, and I can't speak for them, but if I were, my motivation to look at this PR would be quite low, tbh.

@munkybutt
Copy link
Author

The main intent for working on this was to stop start-up lag when using black-d as the backend.
The plugin as it currently is, locks the ui for a few seconds when starting the server, and even longer so if the settings are incorrect and a black-d executable cannot be found.
Trying to run the format command when a server has not successfully started causes another lock of the ui for several seconds.
The error output is not particularly helpful, even when in debug mode, so finding out the cause of the problem was difficult when I first encountered it.
There were further issues of the black-d server continuing to run even after sublime was shut down.
I have attempted to resolve these issues, and the changes were done with the ST4 api, which is not backwards compatible with ST3 hence the renaming to Sublack4.
I am obviously a novice with pull requests, and I have already admitted to the author that I have presented a large and unwieldy set of changes. I have also offered to help make it easier in any way they would require.

I am not sure as to the intent of your message, other than to be snide and condescending.
It is quite remarkable that you would put so much effort into being unhelpful.

munkybutt added 19 commits May 15, 2022 22:17
Plugin now uses sublimes python 38.
This has required the following changes:
- All previous dependencies that were handled automatically by package control are now vendored in sublack.vendor.packages:
-- requests
-- pathlib
-- pyyaml
-- python-toml
This now allows black and blackd to be vendored too, skipping the requirement of black to be setup locally for the plugin to work.
Current list of vendored packages:
-- aiohttp-3.8.1
-- aiosignal-1.2.0
-- async_timeout-4.0.2
-- attrs-21.4.0
-- black-22.3.0
-- certifi-2021.10.8
-- charset_normalizer-2.0.12
-- click-8.1.3
-- colorama-0.4.4
-- frozenlist-1.3.0
-- idna-3.3
-- multidict-6.0.2
-- mypy_extensions-0.4.3
-- pathspec-0.9.0
-- platformdirs-2.5.2
-- PyYAML-6.0
-- requests-2.27.1
-- setuptools-49.2.1
-- toml-0.10.2
-- typing_extensions-4.2.0
-- urllib3-1.26.9
-- yarl-1.7.2
* Created new Standardised  Readme
Removed due to blackd exe requiring an absolute path to a python exe to run. This means installing via local venv and copying to vendored directory will not work
…ver logic

Added vendored python 3.10 and black + black[d] packages and their dependencies.
Re-written black calling logic to use vendored black.py instead of black.exe
Complete re-write of blackd server logic, simplifying from class to module level functions ( for now ) as well as calling blackd.py instead of blackd.exe.
This re-write resolved issue where upwards of 9 python.exes would be spawned and not shutdown after sublime exited.
Fixed bug where blackd process would not shutdown properly when sublime exited
Updated tests to try and get travis builds working
Working on unit test updates and fixing issues found
Import statements are now namespaced rather than top level
All requirements are included in distributed python site-packages folder so removed the duplicate folders from other implementations
vendor.packages contains dependencies for sublack
vendor.python contains python interpreter and dependencies for black and backd execution
* Working on unit tests

getting unit tests functional

* Further work on unit test and simplified logic for getting full black command

* Finally fixed diff logic and test

issue was relating to patched settings

* All unit tests pass!

- cleaned up test imports
- added BlackAll class
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