chaoss / grimoirelab-graal

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

Add support of scancode_cli to CoLic Backend #29

Closed inishchith closed 5 years ago

inishchith commented 5 years ago

Addresses: #27

@valeriocos In reference to your PR. This contains some minor changes. Still, there are some improvements which can be made and tests should be added accordingly. Please do have a look, Thanks :)

Work attributions: @valeriocos 's #28

valeriocos commented 5 years ago

Thank you for addressing the comments @inishchith

inishchith commented 5 years ago

@valeriocos I've updated the PR. I've made the changes wrt. the assumption that exec_path provided would always point to the scancode executable. Please let me know if we're keeping it as the default convention of usage.

Thanks

valeriocos commented 5 years ago

Sorry for the late reply @inishchith . To avoid confusion It is probably better if exec_path points to the file that should be executed (either scancode or scancli.py). Whyexec_pathshould always point toscancode` ?

inishchith commented 5 years ago

@valeriocos This came to my mind as I referred your implementation here and thought if we were planning to do so, now i understand that it was for a quick test. Thanks for the clarification. I'll make the changes

valeriocos commented 5 years ago

perfect, thanks! That branch was a quick test, PR #28 should be better :)

inishchith commented 5 years ago

@valeriocos I've made the changes. Please do have a look I moving ahead to working on tests :)

valeriocos commented 5 years ago

Great @inishchith , thank you for the updates. Please ping when the tests are ready! :)

inishchith commented 5 years ago

@valeriocos I had a thought before starting work on tests.

We are required to accommodate configuring scancli.py before the tests start. So, I was thinking to perform

git checkout -b xxx 8afa686fb71b9540029234e5a40c0572c4457c28

above checkout commit and 1st and 2nd step from this doc in build under before_script under travisci configuration.

Edit: I think we have to use a clone instead of a release as the latest release of scancode-toolkit was on 15th Feb, and scancli was added later ( 5th march ). I didn't realize that I tested my work on a clone, instead of v3.0.0.

Please let me know what you think :)

inishchith commented 5 years ago

@valeriocos I'm not sure why there's no travis-ci and coverage triggered on last push. Can you please check?

Edit: It took some more time to start 🙈

inishchith commented 5 years ago

@valeriocos Please review when you get time Thanks :)

Note:

Edit:

valeriocos commented 5 years ago

I'm on it @inishchith :)

inishchith commented 5 years ago

@valeriocos I've gone through the comments and I see there are improvements needed. I'll work on them and update you accordingly :)

Thanks

valeriocos commented 5 years ago

Great! Thank you @inishchith to check the comments

inishchith commented 5 years ago

@valeriocos I've worked on the requested changes.

Please let me know if I've missed something

Thanks

inishchith commented 5 years ago

@valeriocos Please do have a look when you get time :) Thanks

inishchith commented 5 years ago

@valeriocos Thanks for the review I've updated the doc-string, please do have a look :)