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

Refactor current gensim code by PEP8 #1521

Closed
4 tasks done
menshikh-iv opened this issue Aug 4, 2017 · 7 comments
Closed
4 tasks done

Refactor current gensim code by PEP8 #1521

menshikh-iv opened this issue Aug 4, 2017 · 7 comments

Comments

@menshikh-iv
Copy link
Contributor

menshikh-iv commented Aug 4, 2017

We have a lot of old code which doesn't satisfy PEP8, it's time to fix it!

What we want:

  • PEP8 (without a hard 80-char constraint for long lines)
  • No vertical indents (only hanging)
  • map/filter -> list/set/dict comprehension
  • logging message arguments as method arguments (not direct formatting)
@zsef123
Copy link
Contributor

zsef123 commented Aug 25, 2017

Hi, I'm trying to refactor by PEP8

E731 ( do not assign a lambda expression, use a def these error ) is often seen during refactoring

ex) blas = lambda name, ndarray: scipy.linalg.get_blas_funcs((name,), (ndarray,))[0]

It looks like code writer prefer lambda expresion than def

Do you think you should change lambda expression to def in case of E731 error?

@menshikh-iv
Copy link
Contributor Author

Hi @zsef123, we ignore E731 in gensim, look at this line.

@zsef123
Copy link
Contributor

zsef123 commented Aug 28, 2017

I send PR #1550
but I got some problems.

return buffer[start_index : start_index + nbytes].view(dtype).reshape(shape, order=order)

In case E203 error occurs in pep8 library( https://pypi.python.org/pypi/pep8 ), but it's fine in documentation( https://www.python.org/dev/peps/pep-0008/#other-recommendations )

I think E203 error corresponds to scope delimeter( like if True: )

So First, no change
return buffer[start_index : start_index + nbytes].view(dtype).reshape(shape, order=order)
these line still occurs error in pep8 library but it's fine in docs

Second, line changed to
return buffer[start_index:start_index + nbytes].view(dtype).reshape(shape, order=order)
these line not occurs error in pep8 library and fine too

Which of the two is better?

@piskvorky
Copy link
Owner

piskvorky commented Aug 28, 2017

The second option does not conform to PEP8 (see the "No:" section in https://www.python.org/dev/peps/pep-0008/#pet-peeves).

Basically, the : operator has a lower priority than e.g. +, and so shouldn't have less whitespace because that implies more priority in Python, by convention.

@zsef123
Copy link
Contributor

zsef123 commented Aug 28, 2017

What about ignore F405 and F403 error

F405 : name may be undefined, or defined from star imports: module
F403 : ‘from module import *’ used; unable to detect undefined names

In test_parsing.py

from gensim.parsing.preprocessing import *
error occurs F403
self.assertEqual(strip_numeric("salut les amis du 59"), "salut les amis du ")
error occurs F405 because strip_numeric may be undefined, or defined from star

@menshikh-iv menshikh-iv moved this from TODO to In progress in Code cleanup Sep 5, 2017
@menshikh-iv
Copy link
Contributor Author

Resolved[1] in #1550, will be continued from me (double checking)

menshikh-iv pushed a commit that referenced this issue Sep 5, 2017
* gensim dir PEP8 fixes

* corpora dir PEP8 fixes

* example dir PEP8 fixes

* model/wrapper dir PEP8 fixes

* models dir PEP8 fixes

* parsing dir PEP8 fixes

* scripts dir PEP8 fixes

* similarities dir PEP8 fixes

* summarization and topic_coherence dir PEP8 fixes

* test dir PEP8 fixes

* PEP8 E722 error fixes

* PEP8 fixes

* list slice whitespace PEP8 fixes

* disassemble import *

* Fix symlink

* fix symlink

* fix make_wiki_lemma file

* Replace relative import to absolute

* fix typo

* fix E203 error
@menshikh-iv menshikh-iv reopened this Sep 5, 2017
@menshikh-iv
Copy link
Contributor Author

Continued in #1569

zsef123 added a commit to zsef123/gensim that referenced this issue Sep 6, 2017
…orky#1550)

* gensim dir PEP8 fixes

* corpora dir PEP8 fixes

* example dir PEP8 fixes

* model/wrapper dir PEP8 fixes

* models dir PEP8 fixes

* parsing dir PEP8 fixes

* scripts dir PEP8 fixes

* similarities dir PEP8 fixes

* summarization and topic_coherence dir PEP8 fixes

* test dir PEP8 fixes

* PEP8 E722 error fixes

* PEP8 fixes

* list slice whitespace PEP8 fixes

* disassemble import *

* Fix symlink

* fix symlink

* fix make_wiki_lemma file

* Replace relative import to absolute

* fix typo

* fix E203 error
@menshikh-iv menshikh-iv moved this from In progress to Done in Code cleanup Sep 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

No branches or pull requests

3 participants