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

Add Slyblime package #8000

Merged
merged 1 commit into from
Aug 31, 2020
Merged

Add Slyblime package #8000

merged 1 commit into from
Aug 31, 2020

Conversation

s-clerc
Copy link
Contributor

@s-clerc s-clerc commented Jul 29, 2020

This package integrates with the preexisting Slynk server from the Sly IDE to offer a rich Lisp development experience. There is no similar package.

Copy link
Collaborator

@packagecontrol-bot packagecontrol-bot left a comment

Choose a reason for hiding this comment

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

Automated testing result: ERROR

Repo link: Slyblime
Results help

Packages added:
  - Slyblime

Processing package "Slyblime"
  - ERROR: No valid semver tags found at https://github.com/s-clerc/slyblime/tags for the package "Slyblime".

Copy link
Collaborator

@packagecontrol-bot packagecontrol-bot left a comment

Choose a reason for hiding this comment

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

Automated testing result: WARNING

Repo link: Slyblime
Results help

Packages added:
  - Slyblime

Processing package "Slyblime"
  - WARNING: Creating a readme for your package will help users understand what it does and how to use it
  - WARNING: The binding ['shift+enter'] is also defined in default bindings but is masked with a 'context'
    - File: Default.sublime-keymap
  - WARNING: The binding ['shift+enter'] is also defined in default bindings but is masked with a 'context'
    - File: Default.sublime-keymap
  - WARNING: '.no-sublime-package' is defined. Please verify that it is *really* necessary
  - WARNING: The package does not contain a top-level LICENSE file. A license helps users to contribute to the package.

@s-clerc
Copy link
Contributor Author

s-clerc commented Jul 29, 2020

  • The keybinding things are there for the REPL to work correctly.
  • .no-sublime-package is strictly necessary.
  • There is a COPYING.md file.

Copy link
Collaborator

@packagecontrol-bot packagecontrol-bot left a comment

Choose a reason for hiding this comment

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

Automated testing result: WARNING

Repo link: Slyblime
Results help

Packages added:
  - Slyblime

Processing package "Slyblime"
  - WARNING: '.no-sublime-package' is defined. Please verify that it is *really* necessary
  - WARNING: The package does not contain a top-level LICENSE file. A license helps users to contribute to the package.
  - WARNING: The binding ['shift+enter'] is also defined in default bindings but is masked with a 'context'
    - File: Default.sublime-keymap
  - WARNING: The binding ['shift+enter'] is also defined in default bindings but is masked with a 'context'
    - File: Default.sublime-keymap

@FichteFoll
Copy link
Collaborator

() are illegal characters for identifiers, so not sure what you expect this line to do.

You create a loop in plugin_loaded but then only start it in the connect command despite also scheduling tasks from other commands. This may simply not occur within your plugin lifecycle, but I just want to confirm you thought about this.

Why do you bundle a List.sublime-syntax? Is the default one not satisfactory? Why not submit a PR against the default since you're only targetting recent ST builds anyway? Besides, I strongly suggest to not use the name field and instead have ST infer the syntax name from the file name, as that will be generally less confusing, especially wrt the required .sublime-settings file name.

Is the usage of ë and ö intended?

Other than that, I didn't review eveything because it's too much for me, but the architecture in general seems fine.

@s-clerc
Copy link
Contributor Author

s-clerc commented Aug 23, 2020

() are illegal characters for identifiers, so not sure what you expect this line to do.

Unfortunately, that's a mistake. I've committed a fix, and it will appear in the next tagged release.

You create a loop in plugin_loaded but then only start it in the connect command despite also scheduling tasks from other commands. This may simply not occur within your plugin lifecycle, but I just want to confirm you thought about this.

Yeah, in the case you cite that run function only runs when a completion is generated which only occurs when a connection was established and the loop is running. I do this because nothing can run until we're connected as the async operations all require said connexion. I'm mostly just waiting for

Why do you bundle a Lis[p].sublime-syntax? Is the default one not satisfactory? Why not submit a PR against the default since you're only targetting recent ST builds anyway?

It isn't and I did submit a PR, but to allow new users to start using it as soon as possible I've bundled it. In addition, it's possible that in the future there are more plugin-specific scopes.

Besides, I strongly suggest to not use the name field and instead have ST infer the syntax name from the file name, as that will be generally less confusing, especially wrt the required .sublime-settings file name.

Yeah, I can change that, but I was worried that then there are two Lisp entries in the panel which would confuse people.

@FichteFoll FichteFoll merged commit b408d25 into wbond:master Aug 31, 2020
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.

3 participants