danpovey / pocolm

Small language toolkit for creation, interpolation and pruning of ARPA language models
Other
90 stars 48 forks source link

fix a example pathon file with flake8 style checker #87

Closed LvHang closed 7 years ago

LvHang commented 7 years ago

@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 commented 7 years ago

Looks good. Minor errors are fine. But please accumulate changes from other files into this pull request. When you've done everything we can test and make sure everything still works. As you change things, at least run the run.sh in egs/self_test to make sure it's not broken in a particularly bad way. Dan

On Wed, Nov 23, 2016 at 11:21 PM, LvHang notifications@github.com wrote:

@danpovey https://github.com/danpovey Hi, Dan. Ke Li has already shared the "python style check" task with me.

85 https://github.com/danpovey/pocolm/issues/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

You can view, comment on, or merge this pull request online at:

https://github.com/danpovey/pocolm/pull/87 Commit Summary

  • fix a example pathon file with flake8 style checker

File Changes

Patch Links:

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/danpovey/pocolm/pull/87, or mute the thread https://github.com/notifications/unsubscribe-auth/ADJVu16rUjuxbCeGPQhehHesBt3bZ_Foks5rBRDYgaJpZM4K7RGH .

LvHang commented 7 years ago

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

LvHang commented 7 years ago

@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 commented 7 years ago

Great, thanks. I'll look at this in more detail to-morrow.

On Mon, Nov 28, 2016 at 12:12 AM, LvHang notifications@github.com wrote:

@danpovey https://github.com/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

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/danpovey/pocolm/pull/87#issuecomment-263184992, or mute the thread https://github.com/notifications/unsubscribe-auth/ADJVu5wA9uGnM6gG6pMgvlrJHrCYqfZ0ks5rCmKigaJpZM4K7RGH .

danpovey commented 7 years ago

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

danpovey commented 7 years ago

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 commented 7 years ago

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

wantee commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

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

LvHang commented 7 years ago

@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 commented 7 years ago

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

danpovey commented 7 years ago

OK great, will merge.

On Fri, Dec 2, 2016 at 5:28 AM, Wang Jian notifications@github.com wrote:

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

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/danpovey/pocolm/pull/87#issuecomment-264422910, or mute the thread https://github.com/notifications/unsubscribe-auth/ADJVu5JanunQJ1dhAGd-0xvhEaZwTkt1ks5rD_LMgaJpZM4K7RGH .