-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
pep8/pycodestyle fixes for hanging indents in Summarization module #1202
Conversation
Thanks. When it is ready in due time, could you please link to a PR in pycodestyle repo that highlights error when indent is not a hanging indent? |
gensim/summarization/bm25.py
Outdated
/ (self.f[index][word] + PARAM_K1*(1 - PARAM_B+PARAM_B*self.corpus_size / self.avgdl))) | ||
score += ( | ||
idf * self.f[index][word] * (PARAM_K1 + 1) / | ||
(self.f[index][word] + PARAM_K1 * (1 - PARAM_B + PARAM_B * self.corpus_size / self.avgdl))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
line break should be before binary operator
Please update your pycodestyle
code change accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will update that, should I also change line 51 as in this diff, or is this fine (no arguments on first line).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, It's not a function call so no need for hanging indent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updating this part to something like this diff raises this error E128 continuation line under-indented for visual indent
and using visual indent is not what we want. So should I keep this as it is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And just to confirm the hanging indents are applicable to multi-line return statements too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there should be no E128 error and hanging indent is applicable to all multi-line statements
As per the implementation of check for non hanging indents, you can see the diff here. |
For The current implementation has caused a fase positive on bm25.py line 51 so more work is needed. |
Could you please link to the PR in the pycodestyle repo? |
Thanks for the fixes and improving pycodestyle. We are now closer to achieving #965. |
@tmylk that's cool, but in the meanwhile, please inspect & fix code style manually, before merging PRs. |
@piskvorky The hanging indent requirement has made it harder to automate the style check.
The next steps to automation are merging to pycodestyle and fixing the existing violations. To address your point above - without automation we have to resort to the manual process that is bound to let mistakes slip through as shown in point 2 above. |
Gensim didn't have any hanging indent requirement until recently. Its style was more casual, because there weren't as many contributors. We could rely more on common sense. Fixing existing violations sounds great -- let's start with a consistent, clean slate. And of course, don't merge in anything new that moves us farther away from that. |
Updated the code according to
pycodestyle
. Fixed occurrences of vertical indent to hanging indent.PR in pycodestyle for detecting hanging indents: #632
References: #1081, #1017