chaoss / grimoirelab-graal

A Generic Repository AnALyzer
GNU General Public License v3.0
21 stars 62 forks source link

Add flake8 analyzer to CoQua backend #15

Closed inishchith closed 5 years ago

inishchith commented 5 years ago

@valeriocos Please review and let me know if any changes required :)

inishchith commented 5 years ago

@valeriocos Thanks for the feedback. I've made the changes except for the one related to prettifying the results from flake8. Please do let me know if any changes required ;)

Also i'm not sure why the coverage reports aren't visible after i made the changes 🙈 .

valeriocos commented 5 years ago

To get the coverage report, you should execute the first configuration and then the second one (https://github.com/chaoss/grimoirelab-graal/pull/15#pullrequestreview-216021250)

I run it locally and the lines that are not covered are:

inishchith commented 5 years ago

@valeriocos Thanks for the feedback. I've added verbosity to flake8 analyzer.

To get the coverage report, you should execute the first configuration and then the second one (#15 (review))

This helped me. Thanks ;)

inishchith commented 5 years ago

@valeriocos I've made the required changes and updated the PR. Please do let me know if any changes required ;) Thanks

valeriocos commented 5 years ago

Thank you @inishchith for the PR, I'm going to merge it soon.

Overall the PR looks good to me, I have some doubts about the way of testing the category of a backend (https://github.com/chaoss/grimoirelab-graal/pull/15/files#diff-f72ae87b59ac7e79952fae3be0288df8R145) and whether the worktree_path should be a param of the analyze method (https://github.com/chaoss/grimoirelab-graal/blob/master/graal/backends/core/analyzers/analyzer.py#L37). We could work on them in the future.

Thanks!

valeriocos commented 5 years ago

Merged!

inishchith commented 5 years ago

@valeriocos Thanks for the merge 😄

I have some doubts about the way of testing the category of a backend (https://github.com/chaoss/grimoirelab-graal/pull/15/files#diff-f72ae87b59ac7e79952fae3be0288df8R145)

I took some reference from the previous work here under colic backend tests. which tested the category in a similar way. If you can suggest a better way to do it, I'll make sure to update these :)

and whether the worktree_path should be a param of the analyze method (https://github.com/chaoss/grimoirelab-graal/blob/master/graal/backends/core/analyzers/analyzer.py#L37)

Yeah. i've addressed it over here. I thought about this approach as it wouldn't require iterating through the commits twice [ once in analyzer and once in _post method ]. Let me know if i got something differently here.

We could work on them in the future.

Sure. Thanks :)