Skip to content
This repository has been archived by the owner on Aug 7, 2023. It is now read-only.

Running cli twice on save, improve performance #81

Closed
cgalvarez opened this issue Feb 24, 2018 · 1 comment · Fixed by #83
Closed

Running cli twice on save, improve performance #81

cgalvarez opened this issue Feb 24, 2018 · 1 comment · Fixed by #83

Comments

@cgalvarez
Copy link
Contributor

Right now the cli is being run twice on each file save.

You can check your open processes with ps aux | grep 'codeclimate analyze' or by taking a look at the Atom's dev console (ctrl + alt + i) messages.

I think the approach is just plain wrong. Right now, the command executed is codeclimate analyze -f json path/to/changed/file, and as such, only analyzes the changed file. We're missing some useful and invaluable issues reported by CodeClimate, as duplicated code detection (since this can affect more than one file).

Each time a new save takes place, a new cli executions is spawned. I think it would be preferable to spawn only one cli execution per project (if any of its files is changed), and drop issues of closed files (to avoid errors when calculating range from the textEditor).

With this approach, I think keeping a dictionary with each project root and its associated promise (of its spawned cli execution) would allow to kill the ongoing analysis and start the new one on consecutive savings, without having multiple parallel executions in the background per project.

Also, it would be desirable to debounce the execution, so we could avoid the double current execution (I have no idea of which is causing it).

@cgalvarez
Copy link
Contributor Author

I'm working on a PR to address this issue. Any insight/help will be welcome 😉

With that PR I will refactor the code and will try to improve its maintainability. It's not as good as it could be, according to its own CodeClimate report.

As additional note, this package could get code coverage if switching to mocha instead of jasmine for tests and using a package that I released a few days ago: atom-coverage.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant