package_control_channel icon indicating copy to clipboard operation
package_control_channel copied to clipboard

Pull requests for "Pieces for Sublime" plugin, a plugin that saves code snippets and texts from Sublime text editor

Open moon-bui-pieces opened this issue 3 years ago • 23 comments
trafficstars

  • [x] I'm the package's author and/or maintainer.
  • [x] I have have read the docs.
  • [x] I have tagged a release with a semver version number.
  • [x] My package repo has a description and a README describing what it's for and how to use it.
  • [ ] My package doesn't add context menu entries. *
  • [x] My package doesn't add key bindings. **
  • [ ] Any commands are available via the command palette.
  • [x] Preferences and keybindings (if any) are listed in the menu and the command palette, and open in split view.
  • [x] If my package is a syntax it doesn't also add a color scheme. ***
  • [ ] I use .gitattributes to exclude files from the package: images, test files, sublime-project/workspace.

My package is ...

There are no packages like it in Package Control.

My package is similar to ... However it should still be added because ...

moon-bui-pieces avatar Mar 29 '22 17:03 moon-bui-pieces

My package is Pieces for Sublime

There are no packages like it in Package Control. It is a plugin that saves the user's code snippets and texts to an application, and the user can access their saved snippets from the context menu in Sublime text to insert code/text and increase their workflow productivity

moon-bui-pieces avatar Mar 29 '22 17:03 moon-bui-pieces

  • Why do you add a channel and not simply to one of the repository files?
  • It does sound a lot like SnippetMaker, how is it different?

braver avatar Mar 31 '22 19:03 braver

Hi Braver! Thank you for your response to our pull request. I am more than happy to answer your questions and shed some more light into our Sublime plugin specifically, and our whole software product in general.

To answer your first question:

  • We added a channel because we are choosing to manually host the plugin.
  • Following the guides from step 6 in the Package Control Documentation for submitting a package, we added the URL to “channel.json”.
  • As there was no mention of it in the documentation, I was not aware that you can add to the repository files for manual hosting, so it would be helpful if you can confirm on that matter!

As for the similarities to SnippetMaker, my understanding is that SnippetMaker allows you to save snippets via the Command Palette. After inputting the name and description for the snippet, it is saved as a file under the “User” folder.

While Pieces also allows developers to save snippets, our product focus is quite different in a few ways:

  • We want developers to stay in their flow, so our users can save a snippet from Sublime simply by highlighting the code and right-clicking to save a snippet.
  • We do a lot of ML magic on this snippet - snippets are auto-classified by language and auto-tagged with keywords.
  • Users can re-use their snippets by simply right-clicking on their list of snippets and pasting into their file. Users can also download our desktop app for more functionality, like saving screenshots of code and extracting the code inside via OCR.
  • Our Sublime roadmap is quite ambitious - we want to launch automatic saving of snippets, code auto-complete and more. All of these features are already live in our VS Code plugin by the way, which is a trending top plugin on their marketplace.
  • Pieces is available on IDEs such as Visual Studio Code, IntelliJ and UltraEdit. We also have extensions on Chrome, Safari and Edge for maximum workflow productivity. Just recently, we were featured in a video with FreeCodeCamp if you would like to check us out there!

I hope this was somewhat helpful in answering your questions. If you have any further concerns, please don’t hesitate to reach out to us at [email protected]. We welcome any feedback and would love to hear more from you!

moon-bui-pieces avatar Apr 01 '22 19:04 moon-bui-pieces

Oh, right. We already have a package for Pieces though: https://packagecontrol.io/packages/Pieces

braver avatar Apr 01 '22 19:04 braver

We are aware of that, however that plugin was developed by a previous intern and is made public on Bitbucket, which was not our intention. It does not work as it, and we are not sure how the procedure goes for deleting the plugin for situations like this. It would be great if you can help shed some light on the matter!

moon-bui-pieces avatar Apr 01 '22 19:04 moon-bui-pieces

Alright, we can remove or replace the existing package. If we remove the existing entry and you introduce a new one with the same name everyone who has already installed the package will move to the new one seamlessly.

As per our guideline packages should not have "Sublime" or "For Sublime" in their name: this is the package repository for Sublime Text, all packages here are for Sublime. So rename your package to simply Pieces would be the best for everyone.

I was not aware that you can add to the repository files for manual hosting

Not sure what you mean here? What kind of files?

braver avatar Apr 01 '22 19:04 braver

Alright, we can remove or replace the existing package. If we remove the existing entry and you introduce a new one with the same name everyone who has already installed the package will move to the new one seamlessly.

This is a great solution! Thank you very much!

  • Why do you add a channel and not simply to one of the repository files?

I was referring to this, did I misunderstand your point?

moon-bui-pieces avatar Apr 01 '22 20:04 moon-bui-pieces

Doesn't look like your channel actually provides the package URL so it wouldn't work as is.

rchl avatar Apr 02 '22 18:04 rchl

Your referenced repository.json won't work.

  • details only support URLs to github, bitbucket, or gitlab repositories. If you want to manually host the package, you'll need to provide a different set of keys in the releases list. Furthermore, you'd want to at least specify a homepage url and optionally one for issues.
  • The URLs currently specified in details and readme don't appear to be working.
  • Please adjust the name to "Pieces", as discussed with @braver before. Please also remove the current entry for "Pieces" from p.json in this PR.

FichteFoll avatar Apr 04 '22 08:04 FichteFoll

So just to summarize, what I should be doing right now are:

  • Changing the keys I provided which include version, url, date and the addition of a homepage url. I'm assuming this homepage url is different from the package url. Please correct me if I am wrong
  • That is interesting that it doesn't work, because when we tested the link it appears to be working normally. We'll have it check again tho
  • Changing the plugin name and removing it for p.json I will get to this immediately and get back to everyone on this as soon as possible. Thank you for all the help, I really appreciate it!

moon-bui-pieces avatar Apr 04 '22 13:04 moon-bui-pieces

Your messages.json file is not a valid JSON file as it has trailing comma. Package Control reads that file using python's JSON module so it will not like that. Also, your install.txt file is missing content so maybe that's not really an issue that it's not shown.

rchl avatar Apr 04 '22 19:04 rchl

I don't think you should be shipping Package Control.sublime-settings and Package Control.user-ca-bundle in your package.

rchl avatar Apr 04 '22 19:04 rchl

While I'm at it with fixing the package for push, I'd like to ask what the content encoding/mime-type for the README should be, text/markdown or application/octet stream?

shipping Package Control.sublime-settings

Understood! I have it in there because I wanted the "SubNotify" plugin to be installed with our plugin for notification purposes. Can you suggest a different way of doing this? Would we have to add a "dependencies" key to the packages.json?

moon-bui-pieces avatar Apr 04 '22 22:04 moon-bui-pieces

Understood! I have it in there because I wanted the "SubNotify" plugin to be installed with our plugin for notification purposes. Can you suggest a different way of doing this?

Read https://packagecontrol.io/docs/dependencies

rchl avatar Apr 05 '22 07:04 rchl

@braver @rchl @FichteFoll , I've updated the changes accordingly. You should be able to see it reflect from the channel link, please let me know if there are still any issues. Thank you so much for the help!!

moon-bui-pieces avatar Apr 05 '22 14:04 moon-bui-pieces

Can you instead just declare the dependencies inside your package using the dependencies.json file?

That's because I feel like you are not doing it correctly right now. I don't think you should be defining the dependency SubNotify in your channel file if it's a dependency that already exists and is available in package control. You should only be declaring dependency within your own package entry.

In other words, you should probably remove the packages[0].dependencies key.

rchl avatar Apr 06 '22 18:04 rchl

Can you instead just declare the dependencies inside your package using the dependencies.json file?

Just made the changes, it should be available now!

moon-bui-pieces avatar Apr 06 '22 19:04 moon-bui-pieces

request review @packagecontrol-bot

moon-bui-pieces avatar Apr 11 '22 13:04 moon-bui-pieces

Would you be able to take a look at the updated plugins fixes now and inform me if it works properly @braver @rchl @FichteFoll ? Thank you very much really appreciate it!!

moon-bui-pieces avatar Apr 13 '22 13:04 moon-bui-pieces

Hey I haven't heard back in a while and I was hoping you guys can give me a status update on the situation with the plugin. Is there anything I can do to help speed up the process or make it simpler somehow @braver @rchl @FichteFoll ?

moon-bui-pieces avatar Apr 18 '22 14:04 moon-bui-pieces

No need to mention everyone again and again.


  • The coding style across all files is all over the place and inconsistent.
  • Why duplicate your channel file into the package you're shipping to end users?
  • Why not host the code for your plugin and the channel file on GitHub or Gitlab like everyone else?
  • Since you (target ST4+ and) use py3.8 specifically, why not make use of it in your Python code?
  • official_plugin line 31 should be r"^\s*$" not "^\s*$"
  • Your util method find_platform is redundant, use sublime.platform().

jrappen avatar Apr 19 '22 22:04 jrappen

Sorry for the wait here, I don't frequently review packages anymore and was busy with other things.

The following changes are required before we can merge this:

  • Kind of like @rchl indicated but different: The thing you specified as a dependency, "SubNotify", is a Package and not a dependency (or "library", as they will be called in the future to combat this confusion), which will not work. The package will fail to install because a dependency with that name is not available. If you have a dependency on a different package, you should express that dependency in the readme and have safe-guards in code against it missing (and maybe prompt the user about installing it should it be a hard requirement). Additionally, I'd advise to only specify dependencies in either dependencies.json or repository.json, but that doesn't matter here anyway.
  • In util.py:23 you replace the entire file for each for-loop iteration. You should do it only once after the loop has terminated.
  • When using urllib.request.*, you need to import urlib.request as well, otherwise the code in HTTPClient.py working could just be coincidence (e.g. if the full module has been imported by some other plugin before).
  • The resource that https://storage.googleapis.com/app-releases-59612ba/plugin_sublime/release/Pieces.sublime-package points to should not be changed because otherwise there could be a race condition between when a version update is registered by Package Control and someone installing the newest version of the archive marked as a previous version.

The following are optional suggestions for your code:

  • When opening a file in write mode (i.e. with w), a + has no effect. The file is truncated as it is opened. Seeking to the beginning of the file before closing it in general and closing it explicitly when it is opened through a with block is also redundant. (util.py:49)
  • Building the context menu file path and ensuring its parent folder's existance could be refactored into a reusable function.
  • os.makedirs has an exist_ok parameter that makes it not fail when the directory already exists, making previous checks redundant.
  • During deletion/initial load, instead of rewriting the file with a basic structure, then re-reading the file to add the menu items for the snippets, it would be simpler if you included the basic structure in your "write context menu" function
  • I suggest employing a simple linter such as flake8 for your code to clean up unused imports (and ensure a more consistent code style).
  • Instead of using View.erase and later View.insert, you could use View.replace. (official_plugin.py:56-65)
  • If you already know the contents of a snippet at the time you create the menu entry, you could also use the previously fetched snapshot for the text to insert instead of making another HTTP call. If the snippets are supposed to update frequently in the background, you should update the context menu snippet list periodically anyway (e.g. with a sublime.set_timeout_async chain).
  • As the next step, providing a command palette command with an input handler or opening a quick panel with all snippets for the user to search through seems like a reasonably addition. See here for documentation on the former and here (show_quick_panel) on the latter.
  • When a user has multiple selections and opts to insert text, the usual behavior is to insert the text for all selections and not just the first. (InsertSavedPiecesCommand)
  • Managing packages hosted on GitHub with the "details" feature and adding the package to one of the files inside this repo is generally much simpler as you do not need to update the archive download and manually edit the repository.json. Instead, you would simply create and push a new tag for a new release. You aren't using any feature exclusive to manually-hosted repository.json files.

Well, I suppose lenghty reviews like this are why I eventually got burnt out on doing these regularly, but I hope you learned a thing or two about Python and the Sublime Text ecosystem.

FichteFoll avatar May 09 '22 23:05 FichteFoll

@FichteFoll thank you so much for the detailed response. This is very helpful. I will review and make the necessary updates accordingly.

moon-bui-pieces avatar May 10 '22 13:05 moon-bui-pieces

Let us know when those changes are made, we can re-open this thread then, if needed.

braver avatar Nov 20 '22 15:11 braver