coala / corobo

A bot to help newcomers onboard OS projects! It's awesome!
MIT License
66 stars 103 forks source link

Fix w605 #650

Closed sladyn98 closed 5 years ago

sladyn98 commented 5 years ago

Reviewers Checklist

abhishalya commented 5 years ago

@sladyn98 Please fix this PR.

sladyn98 commented 5 years ago

Yeah @abhishalya can you please verify the above comment, because I am not very familiar with coorbo's internal requirements :) so you could just tell me if I am thinking in the right direction before I go ahead and implement it. Thanks a lot

nvzard commented 5 years ago

@sladyn98 what you need to do is install the latest commit from coala/coala-bears which works without breaking our CI. By doing this we are fixing the dev version of coala-bears that we use, so that our CI won't break if some new addition in coala-bears causes breakage.

sladyn98 commented 5 years ago

@nvzard @nvzard Yeah sure but which branch or release do you want me to add If you want to add a hash we would have to do something like this

git clone [URLTORepository]
git checkout  [commithash]
git clone --branch <tag_name> <repo_url>

If you want to add a tag we could do something like this

abhishalya commented 5 years ago

@sladyn98 You forgot to edit the .travis.yml file, you don't need to move coala, coala-bears requirements manually to the requirememts.txt file. I think it should be fine to use the latest coala, coala-bears since they work fine right now.

sladyn98 commented 5 years ago

@abhishalya IMO we could add a commit or a tag which would make much more sense in pulling the original repos.

abhishalya commented 5 years ago

@sladyn98 No, we don't follow that. For every repo, we need the latest coala, coala-bears so that any fixes in those would reflect for all. And currently both of them work as expected, so we don't need to switch to a particular commit.

sladyn98 commented 5 years ago

Add travis.yml changes.

This PR needs to be fixed as soon as possible.

Yep working on it :)

bkhanale commented 5 years ago

We need to pull the latest coala bears repo from master so we are in dicussions to see whether it works.

Yes, I know that very well. To implement that we shouldn't do modifications to the test-requirements since we'll have to do a lot of changes to Dockerfile which would eventually increase the size of the image. Since we are only using it (coala-bears) on travis we should install them separately (this is what we've already done on travis, we just need to follow the same for semaphore).

abhishalya commented 5 years ago

Yeah, @bkhanale is right here. Sorry if I wasn't very clear earlier. This should help you fix this:

  1. No changes to travis.yml file.
  2. The changes made to pitchfork.py are good, and should be kept.
  3. No changes to Dockerfile.
  4. Change to .ci/semaphore.tests.sh should like travis.yml where we were explicitly installing coala and coala-bears through their github source.
  5. No changes to test-requirements.txt.
abhishalya commented 5 years ago

@sladyn98 Let me know if you would like to work on this, since we need the CI green to be seen at one spot I have to cherry pick your commits into mine. You could keep one commit here, and the other one I could add myself. That would require you to squash current two commits into one (the pitchfork one) and remove the changes of the second commit (the coala one). After which I could pick your commit and then push the other commit into my PR after which we should be able to see the CI green on that PR.

sladyn98 commented 5 years ago

@abhishalya @bkhanale I agree with both of you, but I just spoke to @Makman2 on gitter and he suggested that adding the files to test-requirements would prevent breakages, and the increased size of the dockerfile would not make much of a difference if the build is green again.Thanks for the suggesstions.

abhishalya commented 5 years ago

@sladyn98 It's redundant since corobo doesn't require coala. We are only using it to keep the source clean, creating a docker image with coala is completely unnecessary IMO. I had spoken about this with Makman2 a while back, he had agreed with this.

abhishalya commented 5 years ago

adding the files to test-requirements would prevent breakages

How? We are still doing the same thing.

the increased size of the dockerfile would not make much of a difference

Not the Dockerfile, but the docker image. Its size would be increased and even if size isin't a problem, corobo users wouldn't need coala in docker since either they would already have it installed or they wouldn't need one (if corobo is being used outside coala).

sladyn98 commented 5 years ago

@abhishalya Agreed with you completely :) Couldnt agree more :P

Makman2 commented 5 years ago

That change was requested by me.

Makman2 commented 5 years ago

ack 707a43e 1806316

sladyn98 commented 5 years ago

Done :tada:

Makman2 commented 5 years ago

@gitmate-bot ff

gitmate-bot commented 5 years ago

Hey! I'm GitMate.io! This pull request is being fastforwarded automatically. Please DO NOT push while fastforward is in progress or your changes would be lost permanently :warning:

gitmate-bot commented 5 years ago

Automated fastforward with GitMate.io was successful! :tada:

abhishalya commented 5 years ago

@Makman2 This is not installing latest coala and would rely on coala-0.11.0. As seen from the semaphore CI log

Collecting coala (from coala-bears==0.12.0.dev99999999999999)
  Using cached https://files.pythonhosted.org/packages/cc/74/8b822224ec8b35c4cace3f5768b9573ad33c1a1152f75a61b6c1cb416f78/coala-0.11.0-py34.py35.py36-none-any.whl

This could potentially be harmful, different versions of coala and coala-bears could break many things.

Makman2 commented 5 years ago

If they don't break right now I wouldn't invest more time into it. When the versions get incompatible, the right way would be to specify a newer version in coala-bears's requirements :+1: