Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Add pep8 to Travis #973

Closed
wants to merge 11 commits into from
Closed

[WIP] Add pep8 to Travis #973

wants to merge 11 commits into from

Conversation

souravsingh
Copy link
Contributor

Fixes #965


script:
- python setup.py test
- find . -name \*.py -exec pep8 --ignore=E402 {} + | wc -l
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change ignore to E501 for suppressing "long line" errors.

@tmylk
Copy link
Contributor

tmylk commented Oct 21, 2016

There are definitely pep8 violations so I expected pep8 check to fail. How come it passes?


script:
- python setup.py test
- find . -name \*.py -exec pep8 --ignore=E501 {} + | wc -l
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tmylk he pipelined the errors to show the number of errors instead of raising an error.
@souravsingh please change this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright!! done!

@souravsingh souravsingh changed the title Add pep8 to Travis [WIP] Add pep8 to Travis Nov 24, 2016
@souravsingh
Copy link
Contributor Author

souravsingh commented Nov 24, 2016

Coala installation is creating a problem for Python 2.x systems. Coala is Python 3 only

@tmylk
Copy link
Contributor

tmylk commented Nov 24, 2016

It is ok that coala is only in Python 3. It is sufficient to have pep8 checks in Python 3 only.

@souravsingh
Copy link
Contributor Author

I have added the coala config file. Is there any directories that needs excemption from PEP8 check?( I am thinking of adding /models to ignore list)

@tmylk
Copy link
Contributor

tmylk commented Nov 28, 2016

the pep8bear is not running according to the travis log. Please add it to run in Travis CI

@souravsingh
Copy link
Contributor Author

souravsingh commented Nov 28, 2016

coala bears is now up and running.

@tmylk
Copy link
Contributor

tmylk commented Dec 19, 2016

Can we configure coala to run only on some files? Let's enable it only for the ones that are already clean.
We will clean the rest of the files and add them one by one.

@souravsingh
Copy link
Contributor Author

@tmylk We can configure the coala to run the Bears by making modifications to the config file accordingly.

@nishnik
Copy link

nishnik commented Dec 20, 2016

@tmylk
I have cleaned most of the code.
You can see it here.
I have used a different method, though I would like to mention the pros of the method that I have incorporated (Check .travis.yml):

  • Travis will run for a smaller time. The script checks all the errors only for Python 2.7 (Though we should change it to python 3, I was currently on a system where python 3 was not supported. Apologies)
  • There wont be any coala ci. I dont know whether this is desirable or not.
  • I personally think that using pep8 as it is will help the user spot the errors easily on a local machine.

@souravsingh
Copy link
Contributor Author

souravsingh commented Dec 20, 2016

@nishnik First thanks for the contribution. I will give my two cents on the issue

  1. Coala CI is quite flexible in the sense that you can configure the bears which you need( GitCommitBear, PyCodeStyleBear and so on..) by just a simple commit to the coala config file.

  2. PEP8Bear is essentially PEP8 optimized to run with coala. There is no change in the list of errors, arguments or anything. Only the name of the tools has changed.

  3. Coala requires Python 3 to run nicely, so an extra line for check for Python 3 won't be necessary.

I leave it upto @tmylk to decide on which approach would be nice.

@nishnik
Copy link

nishnik commented Dec 20, 2016

@souravsingh
I completely agree with you on the first two points.

  1. Coala is very flexible, I had attended a workshop where it was demonstrated.
  2. That was a personal statement I made for pep8, I don't like extra dependencies when you can get your work done in a decent manner with the available resources (though I will prefer Coala for other languages (C++, C) but why use a wrapper over pep8 when you can get your work done with pep8)

Sorry that I was not able to get my intentions clearly to you regarding your 3rd point. I have contributed to SymEngine where while I was working on lint checking, it was made sure that we should check for lint errors only once in travis instances, because it is redundant to check for lint errors in every instance (this increases the wait time for getting the ci results).

@tmylk
Copy link
Contributor

tmylk commented Jan 6, 2017

@nishnik Agree that just pep8 is better than Coala as it is easier to use. Also that running linter once per CI is good.

Also thanks a lot for the cleaned code! May I use it as a PR?

May I also ask about the choice of the errors to ignore: E501,E241,W,E731,E402,E265. Are they based on some specific experience in Gensim code?

E501 for line length is clear. Curious about others.

@souravsingh souravsingh closed this Jan 6, 2017
@tmylk tmylk reopened this Jan 6, 2017
@nishnik
Copy link

nishnik commented Jan 7, 2017

@tmylk
Sure! I will send a PR after a few desired changes.
I had ignored warnings(W) and other errors because they couldn't be fixed by autopep8. As of now, I have manually removed (E241, W, E265, E402) will look into E731 and update you.

@souravsingh
Copy link
Contributor Author

@tmylk Closing the PR since the work is finished.

@tmylk
Copy link
Contributor

tmylk commented Jan 8, 2017

@souravsingh Finished by whom?

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

Successfully merging this pull request may close these issues.

4 participants