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 CSS-Intellisense plugin #8939

Closed
wants to merge 3 commits into from
Closed

Conversation

id-sakurata
Copy link

@id-sakurata id-sakurata commented Jun 23, 2024

  • I'm the package's author and/or maintainer.
  • I have have read the docs.
  • I have tagged a release with a semver version number.
  • 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. *
  • My package doesn't add key bindings. **
  • Any commands are available via the command palette.
  • Preferences and keybindings (if any) are listed in the menu and the command palette, and open in split view.
  • If my package is a syntax it doesn't also add a color scheme. ***
  • If my package is a syntax it is named after the language it supports (without suffixes like "syntax" or "highlighting").
  • I use .gitattributes to exclude files from the package: images, test files, sublime-project/workspace.

My package is CSS-Intellisense

My package is similar to "IntelliSense for CSS class names in HTML" from vscode However it should still be added because very useful when developing html page.

Copy link
Collaborator

@braver braver left a comment

Choose a reason for hiding this comment

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

Some problems:

  • sort your package correctly
  • remove the package_control.json and sublime-package.json files from the package, package control will take care of those
  • you shouldn't read settings on plugin load: they're always served from memory can be read at will. that way you also ensure you get the latest version of the setting as users edit them.
  • similarly you will want to update your package on any changes to the settings, also when those don't happen through your specific commands - and not through polling as you're doing with with the cache refresh how. there should be event listeners you could use.

In general I understand the utility (although I wonder if a language service wouldn't be a better approach). However, it seems to be very much set up for a single project. You might want to investigate the use of project settings to allow users to set up "css_folders" for different projects. If you working on many projects those "global" settings will just continue to grow over time.

In its current state I would say the package is a bit too naive to be generally useful (although I can totally see how it works well for you). Maybe you can get others to contribute insights and maybe even code, for instance by talking about it on the Sublime forums and Discord.

repository/c.json Show resolved Hide resolved
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: SUCCESS

Repo link: CSS Intellisense

Packages added:
  - CSS Intellisense

Processing package "CSS Intellisense"
  - All checks passed

@braver
Copy link
Collaborator

braver commented Aug 7, 2024

Looks like you re-formatted the entire JSON file. Please don't do that 😉

@braver
Copy link
Collaborator

braver commented Aug 7, 2024

If you have all your settings covered in your default settings file, it's better to not have those default duplicated in both settings.get calls and the defaults for the class properties. You end up effectively defining your defaults 4 times and they get out of sync (have a close look at your scopes setting for instance).

I wonder if the way you handle settings now, where they're both user settings and temporary caches, doesn't cause issues somewhere down the line. You miss out on Sublime's default handling of automatically updating settings internally whenever users change settings. You seem to end up going deeper and deeper into that rabbit hole, having to call your load_settings method every time your package wants to do anything.

Maybe it makes sense to separate "pure" settings from the caches. That way you can simply use Sublime's api to get and set settings like enabled and auto_refresh_interval. The css_folders and css_files have a more transient aspect to it. But even there I think you might benefit from separating the user setting from the current runtime's caches, and merge the two whenever you want to read those values.

@braver
Copy link
Collaborator

braver commented Sep 3, 2024

Please respond to the feedback to continue the review.

@braver braver added the stale The pull request needs to be updated but has not been within the recent past (2 weeks) label Sep 3, 2024
@braver braver closed this Sep 19, 2024
@braver braver added timeout A pull request needed changes but was not updated in time (2 weeks after becoming stale) and removed stale The pull request needs to be updated but has not been within the recent past (2 weeks) labels Sep 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feedback provided timeout A pull request needed changes but was not updated in time (2 weeks after becoming stale)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants