1138481 - Add .pep8.rc with preferred settings to the repository

Status: RESOLVED FIXED

(Testing :: Firefox UI Tests, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Instead of passing the pep8 configuration to the command directly we should create a .pep8.rc config file in the root folder. We can add all the necessary entries we need to it and simplify the live of contributors in running the modified code through PEP8.

Beside that we would also have to update our .travis.yml so it makes use of the new configuration file:

https://github.com/mozilla/firefox-ui-tests/blob/master/.travis.yml#L33

What our configuration file should contain is:
* max-line-length set to 99
* show-source enabled
* statistics enabled
* count enabled
* excludes for docs

Comment 1

[Aditya Srivastava]

Hey,
I'm an undergraduate student in my first-year of studying Computer Science. It is my request that this bug be assigned to me. I also request to be guided through the procedure of fixing the bug, as it is the first time i'm doing so.
Thanks!

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Hello Aditya! Thank you for the interest in working on this bug. Given your background it kinda sounds like a good one to get started. Before you dive into anything I would advice that you read through the A-Team bootcamp docs: http://ateam-bootcamp.readthedocs.org/en/latest/. With this general information at hand you can fork our firefox-ui-tests repository, create a local clone, and can get started as described in comment 0. Please let me know if at this point you have any questions.

Comment 3

[Aditya Srivastava]

Hey Henrik!
Thanks for assigning the bug to me and for the link. I know some basic algorithmic C and Python programming, and open source is quite new to me.
I tried making the .pep8.rc config file. I looked up the documentation online, but i was unable unable to infer what was meant by "excludes for docs", nor what the docs are. I see an 'exclude' option in the documentation but there are no examples on its use.
Apart from that, i have absolutely no clue how to go about editing the .travis.yml file. I hope i haven't bitten off more than i can chew.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Sorry if the docs part was not that clear. It actually means documentation and in this case it is located in firefox_puppeteer/docs. Everything beneath this folder can be excluded. Regarding the syntax it looks like that it is equivalent to the command line options. So it should be "exclude = %path%". You might want to copy what we currently have in the .travis.yml file. Also for the latter file you should remove all options from the pep8 command and replace it with "pep8 --config %file%". For that just run the "pep8 --help" command, and you will see all the details. I hope that helps to let you continue further. Let me know if something else is missing.

Comment 5

[Aditya Srivastava]

Sir, this is what my pep8 file looks like:
{
[pep8]
max-line-length = 99
show-source enable
statistics enable
count enable
exclude = ./firefox_pupprteer/docs
}

And this is the line in travis.yml that i've changed:
{
-pep8 --config ./pep8.rc
}

Should I push these commits and open a pull request?

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Aditya Srivastava from comment #5)

Hello Aditya,

Thanks for your patch proposal. In the following I have some comments on it.

> [pep8]
> max-line-length = 99
> show-source enable
> statistics enable
> count enable

All key/value pairs need an equal sign, which you missed for 3 of them. It should have given you a failure. Personally I don't think that we have to get those entries listed in this file. Also because we do not make use of them right now.

> exclude = ./firefox_pupprteer/docs

I think you can drop the ´./´ prefix of the path. With the corrected folder name it should work then.

> And this is the line in travis.yml that i've changed:
> {
> -pep8 --config ./pep8.rc

You want to keep the exclude for client here, which is only necessary for travis. 

> Should I push these commits and open a pull request?

Sure, please create a PR for it. It makes it easier to comment on. Once done we prefer to have a text file attached to this bug with only the link to the PR included. When you upload this as attachmnet to this bug you can set the r? for me. Thanks.

Comment 7

[Aditya Srivastava]

I've opened the pull request on the GitHub repo, and set the R? for you. I'll also link to it here:
https://github.com/mozilla/firefox-ui-tests/pull/175

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: github_pull_request.txt

So I will attach the link to the github PR to this bug. It can be used for asking review or feedback from me. Next time when you have fixed the review comments, please edit the attachment and ask for an additional feedback.

f+ given that you are on the right track. It's not ready for review yet, so please fix the mentioned issues as mentioned on the PR.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8608870 [details] [review]
github_pull_request.txt

The updated PR looks fine now. Lets get it in.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

PR got merged as https://github.com/mozilla/firefox-ui-tests/commit/9e20028c7a3df09b162a14667416eed177e35e52

Thanks!