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

Code style fixes to the TFIDF module #1313

Merged
merged 1 commit into from
May 22, 2017
Merged

Code style fixes to the TFIDF module #1313

merged 1 commit into from
May 22, 2017

Conversation

piskvorky
Copy link
Owner

@piskvorky piskvorky commented May 10, 2017

That module is ancient, didn't adhere to the current gensim code style.

@piskvorky
Copy link
Owner Author

piskvorky commented May 10, 2017

@tmylk how far did you get, making the gensim code base style consistent?

@menshikh-iv as Lev probably explained to you, we want to keep the code style consistent. We don't want to merge any new PRs that break this style. Plus we want to fix any existing code style lapses (such as in this PR).

@menshikh-iv
Copy link
Contributor

@piskvorky Where can I read about code style in gensim project? (in developer page I see information about spaces only) ?

@tmylk
Copy link
Contributor

tmylk commented May 10, 2017

@piskvorky There is a hook at every commit that runs the flake8 check on the touched file. That way the code style will improve over time. The hanging indent check is being reviewed by pycodestyle project PyCQA/pycodestyle#632 . It will become a part of gensim's automated checks when it is merged.

BTW This is not a strictly style fix and I would rather omit it. In general having 2 attributes id2word and dictionary with identical function is confusing, but should stay to support backwards compatibility.

@piskvorky
Copy link
Owner Author

I don't think there is a dictionary attribute, so there no duplicity. Or what do you mean?

@piskvorky
Copy link
Owner Author

piskvorky commented May 10, 2017

@menshikh-iv it's pretty much PEP8 and PEP257, except we ignore the strict "80 char" line limit (80 is just a recommendation; if the break would split a line unnaturally, making it hard to read, we use any other limit... 100, 120, whatever).

Also, we use hanging indent (not vertical indent). Both are OK according to PEP8, but we just use hanging indent for consistency and because it's easier to maintain.

I think that's it (@tmylk?)

@tmylk tmylk merged commit 04a3c1a into develop May 22, 2017
@tmylk tmylk deleted the pep8_fixes branch May 22, 2017 21:36
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.

3 participants