Utilize uniqueKey for exec, fix measure bug

AtomLinter / linter-codeclimate

An Atom Linter plugin for the Code Climate CLI

http://github.com/codeclimate/codeclimate

MIT License

10 stars 5 forks source link

Utilize uniqueKey for exec, fix measure bug #64

Closed cgalvarez closed 7 years ago

cgalvarez commented 7 years ago

This PR fixes next issues:

Multiple CodeClimate executions for the same source file (through the option uniqueKey): now there is only one execution running for a given source file (which improves performance and saves device resources). For heavy executions,
Set performance marks based upon the source file's relative path, which allows measuring correctly the concurrent linting of different source files.

Fixes #62.

cgalvarez commented 7 years ago

@Arcanemagus glad to help with those odd bugs that drive us crazy!! :stuck_out_tongue_winking_eye: All your requested changes are commited, sir.

Arcanemagus commented 7 years ago

Glad to see somebody taking an interest in fixing things up, I'd be perfectly fine with giving you write access to this repo if @AtomLinter/linter-codeclimate has no issues with it and you were interested.

cgalvarez commented 7 years ago

Of course! I'm actively using this package right now, so I can assure you any bug that I found or any enhancement that I can think of, I will PR it.

Arcanemagus commented 7 years ago

As I'm implementing this myself in something now, I just thought of something:

Can you think of a reason to keep around the execution timeout with the uniqueKey being implemented? It should be handling killing older runs for us now so the timeout should just be set to Infinity permanently and the option removed as far as I can see.

cgalvarez commented 7 years ago

The unique reason I can think of is that if an exec just hangs up and keeps running indefinitely in the background, it will only be killed by one of two:

The timeout (which we are debating to kill).
Resaving the file which produced the soooo long execution.

But if either the file itself or the command itself is the problem, it could be running indefinitely without the timeout.

Take into account that uniqueKey only kills long-time commands if you re-save the file that executed it.

That's why I initially proposed to set it as an integer, because you can set a numeric timeout that you understand as high enough to be killed. If user provides a timeout of 0, then it´s set to Infinity.

I personally have set it up to Infinity right now, because I´m testing the package thoroughly to fix any bugs I found, but you should consider that in the package you are implementing this.

Arcanemagus commented 7 years ago

Yea, if a process really is running forever though, it will eventually get killed when Atom exits (supposing that it will even die in the first place).