src-d / lookout-gometalint-analyzer

GNU Affero General Public License v3.0
1 stars 5 forks source link

Include langs filter #24

Closed se7entyse7en closed 5 years ago

se7entyse7en commented 5 years ago

This closes #23.

se7entyse7en commented 5 years ago

@smacker I've addressed the issue

smacker commented 5 years ago
  1. We can remove WantLanguage now too.
  2. I believe we also don't need saved variable anymore or at least we need to change a log message. (but I would better remove saved and just return an error if we couldn't save a file. @carlosms wdyt?)
carlosms commented 5 years ago

About 1. Yes, WantLanguage is not needed anymore, we can remove it.

About 2. If we remove saved, can we get the number of changes from Data_GetChangesClient? Or are you proposing to remove also if saved == 0, to run gometalinter even if we received 0 files?

Also about changing tryToSaveTo to return error instead of logging it, that makes sense, but the way it works now also seems sensible. If some files cannot be saved to /tmp, at least we can run the analysis over the rest of the files. I don't have a preference, but in any case it seems unrelated and can be done in a new PR, right?

smacker commented 5 years ago

My problem with the current code is:

Which looks to me incorrect. Before this line was mostly true due to filtering in the same place where saving happens. That's why I think it's related to this PR.

There are different solutions to this problem: changing log message for example as I mentioned before is the easiest one. But it can be solved by some refactoring also which is not really related to this issue.

se7entyse7en commented 5 years ago

I agree that now the behaviour is a bit weird. Especially if fail to save the files, the saved variable, is increased anyway. This may lead to running gometalinter on zero files (worst case). What I would do is to change tryToSaveTo by making it return the error, increment saved only if no error occured. Then we can run gometalinter only if there's at least one file that have been saved.

smacker commented 5 years ago

Sounds like a plan to me 👍