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

Adding Word-Highlighter plugin #7417

Closed
wants to merge 1 commit into from
Closed

Conversation

emanuelen5
Copy link

@emanuelen5 emanuelen5 commented Jan 6, 2019

The plugin is meant for highlighting words or patterns in files (independent of language) in color. I like to use it when reading code to more quickly be able to understand where certain key words (variables) are used, similarly to how I like to use searches. However, many words can be highlighted simultaneously, which thus makes it possible to continue to keep track of the words that are interesting (as opposed to when searching files, where you can only search for one pattern at a time).

See the Readme in the repository for more complete info, as well as a GIF when it is used:
https://github.com/emanuelen5/Word-highlighter

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: word_highlighter

Packages added:
  - word_highlighter

Processing package "word_highlighter"
  - All checks passed

@Thom1729
Copy link
Collaborator

Thom1729 commented Jan 8, 2019

  • It shouldn't be necessary to import word_highlighter.
  • Package names are generally capitalized (e.g. "Word Highlighter"). It's up to you.
  • When your plugin is installed, it will be in the form of a zipfile. You can't modify its contents. If you need to create or store additional files, you can put them in the User package (preferably in a subdirectory).
  • Case-sensitive languages should be a configurable setting, not hardcoded.
  • In re_lang_file = "^" + lang + r'\.sublime-syntax', you need to escape lang: re_lang_file = "^" + re.escape(lang) + r'\.sublime-syntax'.
  • Do not monkey-patch sublime.
  • Command class names should be CamelCase.
  • Instead of raising ValueError in __eq__, return NotImplemented.
  • I'm pretty sure you can't use sublime.load_settings to load a color scheme.

For a package as complex as this, you may want to look at sublime_lib. (Disclaimer: I am the primary author of sublime_lib.) Features that may be of use to you include:

  • ResourcePath, to help manage the color schemes. (JS Custom uses it to manage compiled syntax definitions.)
  • show_selection_panel, to eliminate boilerplate from show_quick_panel (where you're currently using the monkeypatched sublime.INDEX_NONE_CHOSEN).
  • The various enums. Example:
# before
stop_classes = sublime.CLASS_WORD_START | sublime.CLASS_WORD_END | sublime.CLASS_PUNCTUATION_START | sublime.CLASS_PUNCTUATION_END | sublime.CLASS_LINE_START | sublime.CLASS_LINE_END | sublime.CLASS_EMPTY_LINE

# after
from sublime_lib.flags import PointClass
stop_classes = PointClass('WORD_START', 'WORD_END', 'PUNCTUATION_START', 'PUNCTUATION_END', 'LINE_START', 'LINE_END', 'EMPTY_LINE')

@evandrocoan
Copy link
Contributor

How much different is this plugin from:

  1. https://packagecontrol.io/packages/WordHighlight
  2. https://packagecontrol.io/packages/HighlightWords

See also: #7341 Confusing names Word​Highlight versus Highlight​Words
With this addition we would have 3 plugins formerly named:

WordHighlight
HighlightWords
word_highlighter

Ok,, I am getting out of ideias for new names for packages. Let us supposed I got all 3 packages installed, how do I tell which one is which? All have Highlight and Word on the name. One of them has hyphen on the name, the other two...

@emanuelen5
Copy link
Author

Guys, thank you so much for your time.

@Thom1729

  1. It shouldn't be necessary to import word_highlighter.
  2. Package names are generally capitalized (e.g. "Word Highlighter"). It's up to you.
  3. When your plugin is installed, it will be in the form of a zipfile. You can't modify its contents. If you need to create or store additional files, you can put them in the User package (preferably in a subdirectory).
  4. Case-sensitive languages should be a configurable setting, not hardcoded.
  5. In re_lang_file = "^" + lang + r'\.sublime-syntax', you need to escape lang: re_lang_file = "^" + re.escape(lang) + r'\.sublime-syntax'.
  6. Do not monkey-patch sublime.
  7. Command class names should be CamelCase.
  8. Instead of raising ValueError in __eq__, return NotImplemented.
  9. I'm pretty sure you can't use sublime.load_settings to load a color scheme.
  1. Do you the import in sublime_plugin.py? I'll change it to a relative one instead then => from .src import helpers, commands, core.
  2. I wanted to refrain from using spaces in the name since the package cannot anymore be imported in the normal way. Using the other import method also made it harder for me to debug when there were any problems of installing the plugin in Travis CI (which has happened about three in total, and might happen again).
  3. The .sublime-package will be installed in the Installed Packages/ folder, right? I am putting the contents that the user should not have to care about (automatically generated color schemes and log files) in the Packages directory. This is quite nice also when developing since the files then are placed in the same directory as my locally checked out version of the repo. Putting them in the User-directory insinuates (to me) that the files should be modified by the end user, which is not what is intended. Is there something I am missing?
  4. Agreed. I am not using the case-sensitive function currently and consider removing it.
  5. Good spot!
  6. (I feel like these kind of constants should exist in the sublime package)
  7. I will fix this. The Command suffix feels like a bit of overkill though? Easier to find correspondence between the underscore-names and CamelCase ones without it.
  8. Sure, I can change it.
  9. Works like a charm, but might be a hack? The thing is that the .sublime-color-scheme is not pure JSON, so I cannot read it with the JSON package. It however has the same format as .sublime-settings files, which is why the function works so well.

@emanuelen5
Copy link
Author

emanuelen5 commented Jan 12, 2019

@evandrocoan

  1. I would say that it is quite different than WordHighlight. That package is used for selecting the text that you have currently selected or under your cursor. I actually installed this package now since it adds some nice, complementary, functionality that my plugin won't implement.
  2. I had not seen HighlightWords before... Actually, it does almost the same thing as my package (I tried it out). That plugin has some cool functionality that I am missing and vice versa. Is this a problem that they are very similar?

I have considered renaming it to just "highlighter" (or "Highlighter") since I have a vision of coming to a point where you can use the plugin like a smart highlighter pen - not just for words. I will most likely add functionality like coloring sections of code (e.g. grouping functions by functionality) or large portions of text - where the highlight is updated with code changes (shrinks and expands). There are however some limitations in the sublime-API that make it hard to do.

@Thom1729
Copy link
Collaborator

I had not seen HighlightWords before... Actually, it does almost the same thing as my package (I tried it out). That plugin has some cool functionality that I am missing and vice versa. Is this a problem that they are very similar?

It would be best to improve the existing package rather than adding a new one. This isn't a hard requirement, but it makes more sense than having two packages with similar functionality and slightly different feature sets.

Package Control is temporarily down, but I think that this repo contains the HighlightWords package. It does look like it hasn't been updated in a while. I suggest reaching out to the author and seeing if they would be interested in PRs for new features. If they are, then there may be no need for a new package. If they aren't interested in maintaining the package anymore, then they may be interested in someone else maintaining it. If they still want to maintain it but feel that the new features don't make sense for that package, then that might be a good reason to have two packages with different philosophies.

On to the review comments.

I wanted to refrain from using spaces in the name since the package cannot anymore be imported in the normal way. Using the other import method also made it harder for me to debug when there were any problems of installing the plugin in Travis CI (which has happened about three in total, and might happen again).

The casing is what's important, not the spacing. Generally, package names begin with an uppercase letter and dependency names begin with a lowercase letter.

You should use relative imports rather than hardcoding the package name.

The .sublime-package will be installed in the Installed Packages/ folder, right? I am putting the contents that the user should not have to care about (automatically generated color schemes and log files) in the Packages directory. This is quite nice also when developing since the files then are placed in the same directory as my locally checked out version of the repo. Putting them in the User-directory insinuates (to me) that the files should be modified by the end user, which is not what is intended. Is there something I am missing?

I'll have to think about this. Generally, packages have all of their internal code in one place and use User for volatile data. I am not aware of any package that deliberately uses Packages/<package> for volatile data that doesn't also use .no-sublime-package (which is generally bad). I feel as though there should be some significant problem with this, but I can't think of one off the top of my head. I hope that there isn't any problem, because I could use this technique myself for JS Custom.

(I feel like these kind of constants should exist in the sublime package)

Arguably, but patching the shared global sublime package is definitely not a good thing.

At the expense of shilling my own work, “I feel like these kind of constants should exist in the sublime package” is a major reason that sublime_lib exists.

The Command suffix feels like a bit of overkill though?

It's conventional, but I think it works without it.

Works like a charm, but might be a hack? The thing is that the .sublime-color-scheme is not pure JSON, so I cannot read it with the JSON package. It however has the same format as .sublime-settings files, which is why the function works so well.

Use sublime.decode_value(sublime.load_resource('my.sublime-color-scheme')). decode_value recognizes Sublime's extended JSON.

@emanuelen5
Copy link
Author

emanuelen5 commented Jan 15, 2019

It would be best to improve the existing package rather than adding a new one.

I've reached out to the author of that package. I however do not feel like merging the packages for two reasons. First, it would involve a lot of work with writing tests, fixing bugs, etc. Second, I'm not sure if the users of HighlightWords would like me to actually change the functionality of the plugin.

The casing is what's important, not the spacing. Generally, package names begin with an uppercase letter and dependency names begin with a lowercase letter.

I will have to think of a better package name if it becomes accepted into PC.

You should use relative imports rather than hardcoding the package name.

I have fixed this in the sublime_plugin.py entry point. However, the unit tests need to import the package somehow. I do not know why, but the UnitTest package does not seem to support relative imports (perhaps it is rather a limitation in one of the libraries that the plugin uses for testing).

@evandrocoan
Copy link
Contributor

evandrocoan commented Jan 15, 2019

I've reached out to the author of that package. I however do not feel like merging the packages for two reasons. First, it would involve a lot of work with writing tests, fixing bugs, etc.

I use that package. On my fork I added a new feature, just did not bothered in opening a pull requests because most of them are not merged.

Second, I'm not sure if the users of HighlightWords would like me to actually change the functionality of the plugin.

I think I would not like any functionality change on it. If your plugin works different, then, I think it makes sense to be released under a new name.

Also, the package author did not responded about adding a open source license to his project few years back: seanliang/HighlightWords#24 Can it be an open source project?

I do not know why, but the UnitTest package does not seem to support relative imports (perhaps it is rather a limitation in one of the libraries that the plugin uses for testing).

It supports, but you need to know the secret. When I started using the UnitTesting package, I opened an issue about this: SublimeText/UnitTesting#67

To allow relative imports, you need to structure your files like this:

└───YourPackage   (directory)
    │   unittesting.json
    └───tests   (directory)
        └───testing   (directory)
                __init__.py
                test_file1.py
                test_file2.py (which does `from .test_file1 import thing`)

Where unittesting.json has something like:

{
    "tests_dir" : "tests",
    "pattern" : "test*.py",
    "async": false,
    "deferred": false,
    "verbosity": 2,
    "capture_console": true,
    "output": null
}

@randy3k
Copy link
Contributor

randy3k commented Jan 16, 2019

By the way, I guess @emanuelen5 wanted to import the package relatively rather than to import a file under tests relatively.

@evandrocoan
Copy link
Contributor

In that case, there is two alternatives:

  1. If your package has no spaces or special characters on the name like MyPackageGoodName, you can import stuff from it like from MyPackageGoodName import my_cool_thing
  2. Otherwise, you would be required to touch the sys.path and prepend your package root into it, for example:
    import sys
    module = os.path.dirname( os.path.dirname( os.path.realpath( __file__ ) ) )
    if module not in sys.path: sys.path.append( module )
    import my_cool_thing

@emanuelen5
Copy link
Author

[...] @emanuelen5 wanted to import the package relatively [...]

@randy3k
Do you know if this is possible without messing with the Python path? I have currently solved it by importing it through the package name (i.e. import word_highlighter) - but then the package name is hard-coded.

@evandrocoan
Copy link
Contributor

evandrocoan commented Jan 16, 2019

If you would not like to hard code the package name, you could do this:

import os
import importlib

CURRENT_DIRECTORY    = os.path.dirname( os.path.dirname( os.path.realpath( __file__ ) ) )
CURRENT_PACKAGE_NAME = os.path.basename( CURRENT_DIRECTORY ).rsplit('.', 1)[0]

my_package = importlib.import_module( CURRENT_PACKAGE_NAME )
my_package.some_module.some_function()

Or just:

import importlib
my_package = importlib.import_module( __package__ )
my_package.some_module.some_function()

@emanuelen5
Copy link
Author

emanuelen5 commented Jan 16, 2019

I wouldn't like to sacrifice readability of the code. Then I'd actually rather import it by name (leave it as it is).

@evandrocoan
Copy link
Contributor

If you import by name, you hard code the name, so, I did not understood what you want to accomplish.

@emanuelen5
Copy link
Author

@Thom1729

Use sublime.decode_value(sublime.load_resource('my.sublime-color-scheme')). decode_value recognizes Sublime's extended JSON.

Worked swell! Thanks.

At the expense of shilling my own work, “I feel like these kind of constants should exist in the sublime package” is a major reason that sublime_lib exists.

I will migrate to sublime_lib on a branch and see how it works out.
By the way. Is there a plan to integrate these features into Sublime text?

@emanuelen5
Copy link
Author

emanuelen5 commented Jan 16, 2019

@evandrocoan

If you import by name, you hard code the name, so, I did not understood what you want to accomplish.

Yes, correct. @Thom1729 wanted me to remove the hard-coded imports, that was why I was asking for alternatives. However, I do not feel like the alternatives were much better?

@evandrocoan
Copy link
Contributor

For me, both ways should be fine.

This last alternative seems nice:

import importlib
my_package = importlib.import_module( __package__ )
my_package.some_module.some_function()

I just do not recall ever needing to import my own package in my Unit Tests.

Using this alternative, you will not be required to edit/change your Unit Tests code when you rename the package, which could be soon.

Some names I though of:

  1. Lightning
  2. WordEnlightening
  3. Enlightening
  4. EmanuelLightning
  5. ErasmusHighlight
  6. WordLightning

You could open a pool on the forum https://forum.sublimetext.com
image

And presenting your new names suggestions and allowing new name suggestions, and see which gets most voted. Just remember the name WordHighlight was "already taken" by some other package.

@Thom1729
Copy link
Collaborator

The issue with relative imports in tests is a bug and I hope to fix it. It probably won't be in the next couple of days, though, so we'll have to proceed without a fix for now.

Hardcoding the package name is a lesser evil than patching sys.path. My informal standard for hardcoding the package name is "if there's a good reason to do it", whereas for patching sys.path it's "if there's no other option". Getting unit tests to work despite a bug in the testing framework is a good reason to hardcode the package name. You can always switch to relative imports when the bug is fixed.

Thanks @evandrocoan for the insight and possible workarounds.

@emanuelen5
Copy link
Author

I'm gonna close this until I have figured out a better name and fixed the rest of the review comments

@emanuelen5 emanuelen5 closed this Jan 21, 2019
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.

None yet

5 participants