Expose config to non org-wide admins

[bsipocz]

I'm trying to set up pep8speaks for astroquery.

However it's rather inconvenient that it requires an org-wide admin to do any of the configuration rather than someone who has write access to the given repo. E.g. all the CI services, RTD etc.

[OrkoHunter]

Hello @bsipocz, Thank you for trying it out !

PEP8 Speaks is a native GitHub integration and I think as of now installing or removing the integration can only be done by the org admins. However any possible configuration is read from the .pep8speaks.yml file in the repository which I think can be managed by other maintainers as well.

Travis, RTD, etc. do require the org admin to install the service for the org, but other configurations can sure be managed by other maintainers.

So basically, org-admin is required to install/remove any GitHub integration as of now. I will surely ask GitHub support for any possibility of change as the integration is a new feature of GitHub and they are still working a lot on it.

[bsipocz]

@OrkoHunter - Thanks for the quick response. We do have the .pep8speaks.yml, https://github.com/astropy/astroquery/blob/master/.pep8speaks.yml, but the bot doesn't pick up the PRs or respond to commands.

[bsipocz]

Also, this is the settings page. We enabled it organization wide, but something is still not right, and sadly I cannot look into the details without someone higher up in the chain (it may be a useful feedback to GitHub in case they're interested...)

[OrkoHunter]

1. The bot picks the .pep8speaks.yml file from the branch of the PR (similar to how Travis does). Since, the config file was added to the repository very recently, the bot will not be able to find the file for already existing PRs unless they are rebased with the current master branch.
2. The bot is expected to create as little noise as possible. So, it will make a comment only if it finds any pep8 issue.

Would it be okay if I create a PR on the repository for testing it out? I might find a new bug.

[OrkoHunter]

@bsipocz There was indeed a bug (a version mismatch between pycodestyle and autopep8). Thank you for reporting. I've pushed the latest version and now it's working. :)
astropy/astroquery#911 (comment)

[bsipocz]

I assumed that it needed a rebase, so astropy/astroquery#894 was rebased for that purpose :)

Thank you very much for finding the bug and fixing it! I first say pep8speaks in sunpy, and changed my mind about these bots (really disliked the coveralls bot that leaves useless comments for every commit). I think this bot will be very useful for us on the long term to teach all our new contributors our coding guides in a friendly tone.

The plan is to eventually use it in more repos, astroquery is out test case. If everyone is happy with the experiences, I will push to get this enabled for our other packages as well.

[OrkoHunter]

@bsipocz I'm always thankful to sunpy, the current and improved version of PEP8Speaks would not have been possible by the efforts of @Cadair et al.

I can see a lot of astropy repositories in my users list for which pep8speaks is installed, including astropy/astropy. Are you not aware of it? It was installed couple of hours ago only.

[bsipocz]

I believe we have enabled that for everything, but don't yet have the yml file, so in my understanding it should not run yet.

[OrkoHunter]

Oh! But pep8speaks does not need the yml file to work (unlike travis). It will eventually run with the default PEP8 configurations. I'm sorry that it was not clear in the readme. So, I think the org admin will need to take the pain of removing it if it is not intended to run on repositories other than astroquery.

[bsipocz]

Would the config understand a list of errors, or should I list them one at a line?

pycodestyle:
  max-line-length: 100 # Default is 79 in PEP8
  select: E101,E111,E112,E113,E124,E201,E202,E203,E211,E221,E225,E231,E241,E251,E261,E265,E271,E272,E301,E302,E303,E305,E502,E7\
03,E711,E712,E714,E722,E901,E902,W191,W291,W292,W293,W391

[eteq]

@OrkoHunter - Below is what I see. That looks to me like only astroquery is activated? Or am I misunderstanding?

[OrkoHunter]

This is correct ! It's only working on astroquery now. Maybe the "for all" option was accidently clicked before and GitHub sent me a bunch of other repositories' names.

On Fri, May 12, 2017 at 7:03 PM Erik Tollerud ***@***.***> wrote: @OrkoHunter <https://github.com/orkohunter> - Below is what I see. That looks to me like *only* astroquery is activated? Or am I misunderstanding? [image: image] <https://cloud.githubusercontent.com/assets/346587/26000163/2f32371a-3728-11e7-8d96-51b78e51b7f8.png> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#45 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHsTeYLva0pedMumRY4Pp6pasQaEtkiqks5r5F-UgaJpZM4NZCzS> .

-- Himanshu Mishra Fourth Year Undergraduate Mathematics and Computing IIT Kharagpur https://orkohunter.net

[eteq]

Ah, yes, that's exactly what happened: I turned it on momentarily just to see if that would make it speak, but that must have been just when you looked.

[eteq]

@OrkoHunter - it appeared when we turned it on astroquery the first time, nothing happened (until you tried pinging @pep8speaks)... can you clarify what the correct steps are in the future if we decide we want to turn it on in other repos?

[OrkoHunter]

@eteq The reason why it didn't work was a bug which I have mentioned in the comments above. The correct step is simple, you just choose one of the two options in the repository access menu, and then GitHub will start triggering pep8speaks for those repositories. :)

[OrkoHunter]

@bsipocz Sorry for the late response.

Would the config understand a list of errors, or should I list them one at a line?

The select and ignore expects its arguments to be a list in the yml format. Unfortunately the default implementation cannot interpret the comma separate options. (Tested here). Hence the current (and ugly) way to use it, will be

select :
  - E101
  - E111
  - E112
  - E113
  - E124
  - E201
  - E202
  - E203
  - E211
  - E221
  - E225
  - E231
  - E241
  - E251
  - E261
  - E265
  - E271
  - E272
  - E301
  - E302
  - E303
  - E305
  - E502
  - E703
  - E711
  - E712
  - E714
  - E722
  - E901
  - E902
  - W191
  - W291
  - W292
  - W293
  - W391

[bsipocz]

@OrkoHunter - exclude also expects the list to be in yml format, right?

[OrkoHunter]

Yes, I believe that is correct.

On Wed, 28 Jun 2017, 19:57 Brigitta Sipocz, ***@***.***> wrote: @OrkoHunter <https://github.com/orkohunter> - exclude also expects the list to be in yml format, right? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#45 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHsTedF7IcayV0deSGwpiyEiv5_8XnlDks5sImLHgaJpZM4NZCzS> .

-- Himanshu Mishra Fourth Year Undergraduate Mathematics and Computing IIT Kharagpur https://orkohunter.net

[OrkoHunter]

Hello @bsipocz !

Since you have multiple exclude options in the .pep8speaks.yml of astroquery, I would like to inform you that there was an issue ( #52 ) in the exclude config of pep8speaks, which is supposed to be fixed by now! :)

[bsipocz]

Thanks @OrkoHunter for the heads up. I haven't noticed any issues, those file excluded are hardly ever touched (as being copied there from upstream packages or generated on the fly).

[OrkoHunter]

Please re-open if any comment/query has not been addressed. :)