Skip to content

Conversation

@tukusejssirs
Copy link
Contributor

@tukusejssirs tukusejssirs commented Feb 4, 2021

Fixes #28

@kaste kaste changed the title Add info message scope (fix #28) Add info message scope Feb 4, 2021
@kaste kaste merged commit 7b98073 into SublimeLinter:master Feb 4, 2021
@FichteFoll
Copy link
Collaborator

Hm, with this new option, you might as well have luck making it dynamic, i.e. allowing the user to define the code as well as the keywords to match. Would also make the code more streamlined.

I'm assuming that SL can handle dict settings, but I can't think of a reason it shouldn't.

@tukusejssirs
Copy link
Contributor Author

Hm, with this new option, you might as well have luck making it dynamic, i.e. allowing the user to define the code as well as the keywords to match. Would also make the code more streamlined.

I'm assuming that SL can handle dict settings, but I can't think of a reason it shouldn't.

@FichteFoll, what exactly do you mean by making it dynamic? Do you mean that the scopes (info, warning, error or anything else) would be defined in the SublimeLinter settings and the package would read it and use it without hard-coding the scopes (apart from the defaults)?

How would the code be more streamlined?

@FichteFoll
Copy link
Collaborator

Indeed, that is what I meant.

Basically, you would put errors, warnings and infos into a new mapping, generate the regular expression based in the key&value pairs and adjust the following logic accordingly by getting rid of hardcoded strings and replacing them with the dictionary keys.

@tukusejssirs
Copy link
Contributor Author

Indeed, that is what I meant.

Basically, you would put errors, warnings and infos into a new mapping, generate the regular expression based in the key&value pairs and adjust the following logic accordingly by getting rid of hardcoded strings and replacing them with the dictionary keys.

Well, I am partially lost. 😉

You can already replace the default words like this:

"infos": ["Note", "Info", "Nota bene"],
"warnings": ["Warning", "Don't forget", "Potential issue"],
"errors": ["Error", "TNT", "Fatal issue"]

That is, if you define any of the three scopes, the words in the particular scope defined in the defaults won’t be used at all.


Anyway, @kaste, I’ve forgotten to add the infos scope to the example in the README.md; should I create a new PR for that or will you deal with it yourself?

@kaste
Copy link
Member

kaste commented Feb 5, 2021

Well, you can replace the words but not define the "error types". Basically, "errors" is hardcoded in the settings and maps to the error type "error".

A different "API" for the settings would be

   "whatever": {
		"error": [...],
		"note": [...],
		...
	}

so the user could basically invent new "types" just with settings. But it's not very user friendly because a user would have to set a complete dict/mapping in their settings. We don't merge the defaults dict with the user defined dict.

You can also refactor the code ("streamline") without that of course. The only problem with the code is probably the nested word = match.group("whatever") section which obviously doesn't scale because every new type increases the nesting etc.

@tukusejssirs A short PR for the README would be nice.

@tukusejssirs
Copy link
Contributor Author

@kaste, PR created.

As for the suggestion / feature request of @FichteFoll, I’ll leave it up to you two/others, i.e. I won’t implement that as I don’t need it.

@kaste
Copy link
Member

kaste commented Feb 5, 2021

@tukusejssirs Yeah, sure

@FichteFoll
Copy link
Collaborator

But it's not very user friendly because a user would have to set a complete dict/mapping in their settings. We don't merge the defaults dict with the user defined dict.

Imo that is fine for a "small" list like this, but I also personally don't need it, which is why staying at the status quo, if even for backwards compat, is probably the way to go.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Would it be possible to add additional info scope?

3 participants