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

pep8/pycodestyle fixes for hanging indents in Summarization module #1202

Merged
merged 4 commits into from
Mar 27, 2017

Conversation

SamriddhiJain
Copy link
Contributor

@SamriddhiJain SamriddhiJain commented Mar 10, 2017

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

@tmylk
Copy link
Contributor

tmylk commented Mar 12, 2017

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?

/ (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)))
Copy link
Contributor

@tmylk tmylk Mar 12, 2017

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.

Copy link
Contributor Author

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).

Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor Author

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?

Copy link
Contributor

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

@SamriddhiJain
Copy link
Contributor Author

As per the implementation of check for non hanging indents, you can see the diff here.
For hanging indents there are two conditions, no argument in first line and continuation line check. The latter is already implemented in pycodestyle 1. So I have implemented the first check (which occurs in the case of visual indents), when arguments are present on the first line.
Although I have submitted the pull request, but the tests failed because most of the tests and code itself are in vertical indents format, that is why I think its difficult to merge these changes in their branch, unless the tests are also carefully changed.

@tmylk
Copy link
Contributor

tmylk commented Mar 12, 2017

For pycodestyle PR it should be an optional new check which is off by default. That will cause no issues with existing tests.

The current implementation has caused a fase positive on bm25.py line 51 so more work is needed.

@tmylk
Copy link
Contributor

tmylk commented Mar 20, 2017

Could you please link to the PR in the pycodestyle repo?

@SamriddhiJain
Copy link
Contributor Author

@tmylk Link for PR in pycodestyle: 1.

I tried to keep this test in default ignore by editing in pycodestyle.py, setup.cfg, test_api.py, but it still runs. Is there any documentation which can be used to implement this?

@tmylk
Copy link
Contributor

tmylk commented Mar 27, 2017

Thanks for the fixes and improving pycodestyle. We are now closer to achieving #965.

@tmylk tmylk merged commit fe49d98 into piskvorky:develop Mar 27, 2017
@piskvorky
Copy link
Owner

piskvorky commented Apr 9, 2017

@tmylk that's cool, but in the meanwhile, please inspect & fix code style manually, before merging PRs.

@tmylk
Copy link
Contributor

tmylk commented Apr 10, 2017

@piskvorky The hanging indent requirement has made it harder to automate the style check.

  1. Gensim is the only Python open source project that enforces the hanging indent. It's not a trivial common practice. A specific extension is in the process of being added to pycodestyle as described in this PR.

  2. Even in gensim codebase there is code from years ago that doesn't comply with hanging indent. (237 lines)

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.

@piskvorky
Copy link
Owner

piskvorky commented Apr 10, 2017

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.

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.

None yet

3 participants