pep8 python files - Githubissues

bgruening commented 6 years ago

Would it be welcome to use pep8 for the python files? I was trying to look at the code to see why it takes so long, but it is hard to read at this point, Would pep8 be a welcome contribution?

Maux82 commented 6 years ago

yes, it would be a good contribution. :)

Maux82 commented 6 years ago

@bgruening I have pushed a version that should fix some of the issues that you pointed out in #35 .

the use of mzML is not yet ready and tested. My plan, is to migrate to python 3.0 when I have a stable version .and then using the new version of pymzML. Any inputs/suggestion to migrate to python 3.0 is more then welcome.
you suggested to use boolen in all my flags and I changed some of them. what about the flag set in CLI ? (e.g match_filter ) ? For the user is much more easy to insert a 0/1 instead of True/False . What do you think ?
you can test it using the input file in the sample folder. you can find the raw file here

bgruening commented 6 years ago

@Maux82 do you want to support py2 and py3 or is it ok to go fully to py3? Only py3 would be easier.

For the user is much more easy to insert a 0/1 instead of True/False. When does a user need to insert True/False?
it would be cool if you could include the test as part of the travis testing.

bgruening commented 6 years ago

Thanks for working on it!

bgruening commented 6 years ago

@Maux82 https://github.com/compomics/moFF/pull/36 is using pure py3 if you like to do it that way.

Maux82 commented 6 years ago

yes I bit of support it would be great and I would go fully to py3. @antortjim can you also help us on this ?

I meant --match_filter is a flag but the user insert a int (0/1) and I use it as int without cast it to boolean. Do you think I have to cast them (those kind of input 0/1) to boolean ?

I will check how to can I include the test in the travis testing. I never used used travis-CI.

bgruening commented 6 years ago

@Maux82 check out https://github.com/compomics/moFF/pull/36 :)

I meant --match_filter is a flag but the user insert a int (0/1) and I use it as int without cast it to boolean. Do you think I have to cast them (those kind of input 0/1) to boolean ?

Can you point me to the argparse code for this?

bgruening commented 6 years ago

I will check how to can I include the test in the travis testing. I never used used travis-CI.

It's really worth to spend one or two days on this. It will give you a good feeling that this @bgruening jerk is not destroying your tool if you merge his PRs :)

bgruening commented 6 years ago

@Maux82 if you are talking about https://github.com/compomics/moFF/blob/502d9040c5f9c31549f26a7ec5a7624150931e02/moff_all.py#L125

please read https://docs.python.org/3/library/argparse.html#action especially the store_true

bgruening commented 6 years ago

See here: https://github.com/compomics/moFF/pull/37

antortjim commented 6 years ago

Hi @Maux82! Yes I would be happy to help. I went through the code in moff.py and changed it to make it more readable, even though at some point I broke it somewehere and discarded the work. I will work on it from September 11th, as currently I am traveling! I think one of the priority improvements that can be done to the code is getting the variable names correctly, as in some cases it needlessly changes 2-3 times. What do you think? :smile:

Maux82 commented 6 years ago

See here: #37 @bgruening I used the store_true for all boolean inputs. I don t merge your PR because I have already added on my last push.

To notice that if the user use a config file istead of the CLI must specify these boolean parameters with True/False. https://github.com/compomics/moFF/blob/32fd4a69b914b7205dd0ec9b0b42794f87acad7e/moff_all.py#L57

Now, moFF works only in py3.5 . From your initial #36 I have changed a few things to make it works such as :

https://github.com/compomics/moFF/blob/32fd4a69b914b7205dd0ec9b0b42794f87acad7e/moff.py#L946

I still need to test the current version on Linux but I think it should be straight forward.

Other improvements suggested by you in #35 in mbr module hopes will come soon.

Maux82 commented 6 years ago

@bgruening
https://github.com/compomics/moFF/issues/34#issuecomment-417112458

I have set the Travis-CI, Can I modify the current travis.yml file to test my code or I am gonna broke something ? e.g the availability in bioconda. ?

thanks for the big help !

bgruening commented 6 years ago

To notice that if the user use a config file istead of the CLI must specify these boolean parameters with True/False.

Is that a problem? Honestly, I would drop the config file option completely.

You can change travis as you like, it has no influence on bioconda and you can not break something :)

compomics / moFF

pep8 python files #34