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

Make the include setting opt-in, and enable the extension to work globally otherwise #72

Open
willdhorn opened this issue Apr 17, 2023 · 14 comments
Assignees
Labels
enhancement New feature or request

Comments

@willdhorn
Copy link

willdhorn commented Apr 17, 2023

As mentioned in #69, the include and exclude settings weren't actually wired up previously, so the default behavior was to enable the extension globally. IMO, include should be undefined or an empty list by default - which would enable the extension globally1. Then if the user wants, they can limit which files types the extension considers by putting file types (or languages as mentioned in this comment) in the includes setting. This makes more sense than than shipping the extension with a predefined list of (mostly web-dev related) file types that it works with and forcing users to add the file types that are specific to how they use vscode.

There are a couple reasons I think this is an improvement:

  • This was the default behavior (unintentional or not) prior to the recent changes, and implementing this will revert back to that behavior for everyone that didn't add the include option to their settings, but while still keeping the benefit of actually having the setting wired up for those that want to use it.
  • It allows users to enable the extension globally without using **/*, as the readme explicitly warns against2, or, rather absurdly, adding every file extension they are likely to work with
  • As mentioned above, the defaults seem to be aimed primarily at web-dev, but vscode is being used more and more as a general purpose text editor/IDE that it's not unlikely some one programming in python or go installs the extension just to see nothing happen. (this would be made more confusing by Default settings not appearing on installation? #71 and not seeing a setting to modify if they edit their settings with json)

Footnotes

  1. It probably makes sense to have a default exclude list to avoid slowing down scanning large files that are not relevant

  2. I'm making the assumption that the performance impact mentioned of using **/* is different than whatever the extension was doing before the recent changes

@jgclark
Copy link
Owner

jgclark commented Apr 17, 2023

Thanks for these good observations and suggestions, which I agree would make for smoother on-boarding.

The comment about performance impact of **/* is inherited from the original author, and AFACIT hasn't been checked. Given they didn't fully test this aspect of the extension, I'm not sure we should now give that too much credence.

As best I can tell, VSCode does use the provided default settings for include and exclude on a new installation, and it shows them in the visual settings editor. So for exclusions, that is:
"todohighlight.exclude": [
"/node_modules/",
"/QuickLook/",
"/dist/",
"/build/",
"/.vscode/",
"/.github/",
"/_output/",
"/.min.",
"
/*.map",
"/script.js",
"
/.next/**"
]

@alkatar21 and @elazarcoh are now helping maintain the extension, and are working on other improvements at the moment. So I'd like their opinion before proceeding.

@elazarcoh
Copy link
Collaborator

I'm having similar thoughts.
I also don't like the default value of the include to be a close set of file types (for instance,it doesn't include python and cpp, and would confuse newcomers).
My suggestion is to make special treatment in case of **/. (if that really is a performance issue, which I really doubt is relevant to our use case. For me it sounds more relevant in case of matching hundreds of files etc, but I don't really know).

@alkatar21
Copy link
Collaborator

I am currently very busy, but in a few weeks I have some time. I would also prefer it if include would be empty by default and the extension works globally and we possibly extend the exclude list with typical folders you don't want to search (for some common languages).

@ygoe
Copy link

ygoe commented Apr 20, 2023

I just looked at the code and believe that **/* should be used if the array is empty. So if I haven't set the array in my setting, why is the highlighting not applied to all files then? I don't understand that.

@willdhorn
Copy link
Author

willdhorn commented Apr 20, 2023

When you don't set a value for the setting, it uses the default value set by the extension, which is defined here.

However, even when I set the setting to an empty list, a string, or even null, it doesn't highlight anywhere, so there's either an issue with the getPaths function or something else about the way it's handling the setting that's causing it to not evaluate to the '{**/*}'.

On the other hand, I went against the advice or the original author and manually set "todohighlight.include" to ["**/*.*"] and have not noticed any kind of performance issue, which makes sense given that was the default behavior before. So if you want to enable the extension to work globally, there doesn't seem to be any problem with doing that like what that warning alludes to.

@ygoe
Copy link

ygoe commented Apr 20, 2023

Yes, I came to the same conclusion now. I want my editor to highlight TODOs, no matter where. And this is a text editor, not something meant to deal with binary data. Opening very large files probably causes performance issues in VSCode before this extension has an effect on it.

@jgclark jgclark self-assigned this Apr 20, 2023
@jgclark jgclark added the enhancement New feature or request label Apr 20, 2023
jgclark added a commit that referenced this issue Apr 20, 2023
@jgclark
Copy link
Owner

jgclark commented Apr 20, 2023

I'm releasing 2.1.0 beta here to test these changes to settings:

  • todohighlight.include is now dropped.
  • todohighlight.includedLanguages is now included as an optional list of languageIds where highlighting will be turned on (e.g. ["typescript", "go"]). If empty, then all open files will be highlighted.
  • todohighlight.exclude is now called todohighlight.excludedFiles. It remains an array of strings of glob pattern that defines files and folders where highlighting will not be turned on.

Test it by getting vscode-todo-highlight-2.1.0.vsix and on the extensions sidebar click the ... menu at the top right and select "Install from VSIX...".
vsix-install@2x
You will then need to re-load the VSCode window.

cc @willdhorn @elazarcoh @alkatar21 @ygoe

@jgclark
Copy link
Owner

jgclark commented Apr 23, 2023

This is a quick nudge to @willdhorn @elazarcoh @alkatar21 @ygoe hoping that one or more can test this new .vsix (which hopefully contains the updated documenation) so that I can know whether to release it or not.

@alkatar21
Copy link
Collaborator

I am currently very busy, but I have at least briefly looked over it and left feedback.

@jgclark
Copy link
Owner

jgclark commented Apr 23, 2023

I had changed include setting to includedLanguages in response to the request by @RyanTheTechMan in #69 at #69 (comment).

In #75 @alkatar21 commented that this stops just selecting a few files or folders, which is true.
So, do I revert this change back to using globs/file paths, but still allowing for the 'all files' case by leaving include empty?
Or do I allow both include (as above, and essentially no change from v2.0.4), but also allow anything mentioned in includedLanguages?

@alkatar21
Copy link
Collaborator

I would just do include for files for now and include: [] should then behave exactly as it does now in the includedLanguages branch, i.e. allow all files that are not excluded.

I am unsure if includedLanguages should be supported for one person, as it is probably rather confusing for most users. If there is more interest in it, you can always support it additionally later. But then we should see if there is something better than "items": {"type": "string"} which supports language ids autocompletion.

@willdhorn
Copy link
Author

I've been using the new .vsix and it's working without having to override anything in my settings. Thanks!

@ygoe
Copy link

ygoe commented Dec 21, 2023

Any update here? Where do I get that beta version? I see that there was no update since then. Does it have the language selection or the file selection now?

@elazarcoh
Copy link
Collaborator

Any update here? Where do I get that beta version? I see that there was no update since then. Does it have the language selection or the file selection now?

https://github.com/jgclark/vscode-todo-highlight/blob/jgclark%2Fissue72/vscode-todo-highlight-2.1.0.vsix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants