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

fix a example pathon file with flake8 style checker #87

Merged
merged 1 commit into from
Dec 2, 2016

Conversation

LvHang
Copy link
Contributor

@LvHang LvHang commented Nov 24, 2016

@danpovey
Hi, Dan. Ke Li has already shared the "python style check" task with me. #85
I have already install the vim-flake8 plugin.
This is an example file which I modified according to the suggestions of flake8. It still has one problem in line 11. The flake8 says "line too long (80 > 79 characters)". Just one character. I'm not sure whether I need to change it.
Please let me know whether what I do is in line with your ideas. If it's okay, I will continue.
Thanks.
Hang

@danpovey
Copy link
Owner

danpovey commented Nov 24, 2016 via email

@LvHang
Copy link
Contributor Author

LvHang commented Nov 24, 2016

Get it, I will accumulate changes with command like "git rebase".
I will test after change each file.
Hang

@LvHang
Copy link
Contributor Author

LvHang commented Nov 28, 2016

@danpovey
Hi Dan,
I think maybe I have fixed all the python files in 'pocolm' according to the suggestions of 'flake8'.
After I modify each file, I run the run.sh in egs/self_test. I'm sure it works so far.
Maybe you can conduct further tests. I will modify them if there is a problem in the file or you think it need to be changed.

The files still have some alerts when you check them with 'flake8':
(The first four kind warning is very limited)

  1. E402 module level import not at top of file. Because we import the function from pocolm_common or others. We need make sure the scripts/internal is on the pythonpath. So this kind of alerts will still exist.
    I think we can leave it there.
  2. F821 undefined name which I told you in the email.
    I think we can leave it there.
  3. F841 local variable is assigned to but never used. I'm not sure whether you need it in the further.
    So I leave it there.
  4. E114 indentation is not a multiple of four(comment). It only appears in 'word_counts_to_vocab.py'. When successive multiple single-line comments(# ) appears in the middle of multiple raws, the problem may occurs.
    I leave it there for easy understanding the content of comment.
  5. Most of the remaining alerts is E501 line too long. For this kind of alerts, I have no idea about how to judge whether it need to change. If you can give me some suggestions, I will appreciate it. And I will fix it.

Thank you for your patient and guidance.
Hang

@danpovey
Copy link
Owner

danpovey commented Nov 28, 2016 via email

@danpovey
Copy link
Owner

Actually, @wantee, do you have any time to look and see that what he did was reasonable, and maybe run some of the tests?

1 similar comment
@danpovey
Copy link
Owner

Actually, @wantee, do you have any time to look and see that what he did was reasonable, and maybe run some of the tests?

@wantee
Copy link
Contributor

wantee commented Nov 28, 2016

OK, Great. I will take a look at this.

@wantee
Copy link
Contributor

wantee commented Nov 30, 2016

I ran tests on swbd, it looks good.
But I still get some errors with flake8 besides those listed by Hang:

F401 'gzip' imported but unused

E121 continuation line under-indented for hanging indent
E126 continuation line over-indented for hanging indent

Is that due to different version of flake8? mine is

$ flake8 --version
2.4.1 (pep8: 1.5.7, mccabe: 0.2.1, pyflakes: 1.0.0) CPython 2.7.11 on Linux

@LvHang
Copy link
Contributor Author

LvHang commented Nov 30, 2016

@wantee @danpovey
Hi wnatee,

  1. For F401, I'm sorry I forget it. The command 'import gzip' appears in 'try ... exception ...' with annotation. I'm not sure whether I need to delete them.
  2. For E121/E126, In my system, this kind of warning is not exist after I modify. Could you show me a file name which contains the problem?

My flake8 version is:
3.2.1 (pyflakes: 1.3.0, mccabe: 0.5.2, pycodestyle: 2.2.0) CPython 2.7.9 on Linux
I think maybe the different between 'pycodestyle' and 'pep8'. In Readme file, vim-flake8 said it supersedes both vim-pyflakes and vim-pep8. Could you tell me how you specify pep8 rather than pycodestyle? Thanks a lot.

Bests,
Hang

@wantee
Copy link
Contributor

wantee commented Dec 1, 2016

If we don't use the gzip in a file, you can remove the import code.

You can find my full log at http://pastebin.com/Mkh71Z9K. I ran flake8 scripts/ to produce that.

@LvHang
Copy link
Contributor Author

LvHang commented Dec 1, 2016

@wantee
Ok, Get it.
I will try. Thanks a lot!
Hang

fix all the python files in pocolm with flake8

fix the import gzip
@LvHang
Copy link
Contributor Author

LvHang commented Dec 2, 2016

@danpovey @wantee
Hi Dan and Wang,

I fix the "import gzip" in files.
But I leave the E121/E126 there. Because my flake8 version is higher. I have no idea to downgrade the version. (On the github of pycodestyle, the pep8 and pycodestlye are the same, pep8 was renamed to pycodestyle to reduce confusion).

I conduct the test on egs/self_test. Maybe you can conduct further tests.
Please inform me, if there are other problems or something I can help.

Thank you for your guidance.
Hang

@wantee
Copy link
Contributor

wantee commented Dec 2, 2016

Thanks, I ran the swbd test, it works well. You could merge this PR, Dan, if you think the remaining errors are acceptable.

@danpovey
Copy link
Owner

danpovey commented Dec 2, 2016 via email

@danpovey danpovey merged commit f86d37a into danpovey:master Dec 2, 2016
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