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

Test updates for #72 #75

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Test updates for #72 #75

wants to merge 3 commits into from

Conversation

jgclark
Copy link
Owner

@jgclark jgclark commented Apr 20, 2023

#72

Copy link
Collaborator

@alkatar21 alkatar21 left a comment

Choose a reason for hiding this comment

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

I haven't looked at it in detail now, but I would suggest to leave the names of the settings as they were before, at least exclude should not change in my opinion. For include I understand the idea of not using the previous setting so that everyone benefits from the changes, however I would not use languages as include settings. What about usecases where the extension should only run in specific folders for example? What about custom files that are not assigned to any language? That would limit the usability of the extension.

There may be other things that I didn't notice at first glance.

I have not tested the settings itself, but TODO: is displayed again without any settings present, that works.

src/util.js Outdated Show resolved Hide resolved
src/util.js Outdated Show resolved Hide resolved
Comment on lines +109 to +117
// check if we have a list of languages to be used for. If not: use on all languages.
const thisFileLangId = activeEditor.document.languageId;
console.log(`thisFileLangId: ${thisFileLangId}`);
const includedLanguages = settings.get('includedLanguages') || [];
console.log(`includedLanguages: ${String(includedLanguages)} (${typeof includedLanguages})`);
if (includedLanguages && includedLanguages.length > 0 && !includedLanguages.includes(thisFileLangId)) {
console.log(`updateDecorations not wanted on lang ${thisFileLangId} for file ${activeEditor.document.fileName}`);
return;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this not in the function that checks if the current file should be used?

Copy link
Owner Author

Choose a reason for hiding this comment

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

There was a reason at the time, but I now forget why.
But it may not be relevant now, depending how the discussion on which type of include setting to use.

jgclark and others added 2 commits April 23, 2023 20:58
Co-authored-by: alkatar21 <61387986+alkatar21@users.noreply.github.com>
Co-authored-by: alkatar21 <61387986+alkatar21@users.noreply.github.com>
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.

2 participants