SoftwareMarbles / lazy

Hackable Coding Assistant
http://getlazy.org
MIT License
1 stars 0 forks source link

Same message for no errors and no engines #61

Closed ierceg closed 7 years ago

ierceg commented 7 years ago

I get "no linter warnings" info even when no linter engines ran for the file. Here's an example: https://www.dropbox.com/s/vo9817ily7hgvrn/Screenshot%202017-01-11%2018.05.55.png?dl=0)

There should be two infos:

  1. Wo hoo - no linter warnings.
  2. Hey no linters for this

Previously I would get "no engine registered for ". This is especially important in Atom where installing a plugin can change the declare grammar of the language (so instead of "JavaScript" it becomes "Babel ES6 JavaScript" and our linters stop working but what we report is that the runs are now "clean". However I never push this update (bravo me!) so now it's useless as we no longer have noRegisteredLanguageEngines in postprocessing engines.

Also, we can leave off line and column for such generalized messages and they will show just fine, only not on any specific line. This is how I used to create the warning.

              if (body.noRegisteredLanguageEngines) {
                results.push({
                  type: 'Info',
                  text: `No specific engine registered for [${grammar}] grammar`,
                  filePath: editor.getPath()
                });
              }
neboysa commented 7 years ago

The problem here is that an engine can actually be registered for a given language, but it may not really lint it. For example, postprocessor engine is registered specifically for all languages that support \\, \* *\ and # style comments, but it doesn't actually analyze them (doesn't lint them) - instead it just postrpocesses messages from previous engines.

Therefore, each engine needs to report whether it has analyzed the given file, or not. If at least one engine reports that it has analyzed the file, then we now that the file was linted.

Engine can report this by returning a new status property in addition to existing warnings property, like this:

{
  status: {
    codeChecked: true
  },
  warnings
}

If status contains codeChecked: true it means it has analyzed the file.

ierceg commented 7 years ago

@neboysa I understand. But why change lazy in #66 instead of doing everything in the postprocessor or reduce engines? Maybe this will lead to a proliferation of engines? But I think that it might be sufficient to have one standard postprocessor engine with whatever we want it do. And then others can hack their own postprocessors if they want different behavior.

neboysa commented 7 years ago

@ierceg - yes, I had a same thought, but I wanted to avoid special cases. Moreover, this approach brings a standard way for every engine to report its status to lazy - at a moment, only codeChecked is reported, but in future there may be more. More precisely, response of every engine now has two parts: status which will never be visible to end users (only internally to lazy), and warnings which is something that engine intends to show to end user.

ierceg commented 7 years ago

I wanted to avoid special cases.

Could you elaborate on this? I'm not sure which special case you refer to.

Ok re status but I think that less lazy knows about such things the better. So we start using status to return stuff from engines and then... what happens? lazy will have to be adapted to each new property coming in the status and that means hacking lazy when only engine could have been hacked. Ideally lazy would only know its output format and remove everything else from it (so if it gets { warnings, status } from the pipeline, it checks it against its know format and removes status).

Also codeChecked looks pretty special-case-y to me (and in the same vein so does the knowledge of languages in lazy, extraction of warnings, etc.)

neboysa commented 7 years ago

I was looking it at the other way around: lazy is the one that defines what it needs to see in status, and engines have to comply. Whatever else engines put in status, will be ignored by lazy, and in no case will engine A be aware of status of engine B.

On a special case: somebody has to know if the file was linted by some engine, or not, so the fact can be reported to user. I don't think it is a good approach to have a special engine that will have this knowledge - somehow, it is close to my mind that lazy should be aware of these things, rather than any specific engine. So, lazy asks each and every engine "Have you linted this file?" and engine replies with yes/no. Or, if you wish - each engine sends a status describing what it has done with the file (e.g. checked code) and lazy decides what to do with that information.

neboysa commented 7 years ago

I think this is better approach than having lazy assume that the engine has processed the file in certain way (checked the code, or not) based only on association of the engine with a language.

ierceg commented 7 years ago

Ok, let's go with this then. If we see that lazy is getting "fat" we'll revisit. It's an implementation detail anyway but what I worry about is setting precedents (even for us) rather than allowing hacking to be done easily through engines.