Expose config to non org-wide admins about pep8speaks HOT 21 CLOSED

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.

from pep8speaks.

@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.

from pep8speaks.

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...)

from pep8speaks.

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.

from pep8speaks.

@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)

from pep8speaks.

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.

from pep8speaks.

@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.

from pep8speaks.

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.

from pep8speaks.

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.

from pep8speaks.

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

from pep8speaks.

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

from pep8speaks.

from pep8speaks.

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.

from pep8speaks.

@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?

from pep8speaks.

@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. :)

from pep8speaks.

@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

from pep8speaks.

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

from pep8speaks.

from pep8speaks.

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! :)

from pep8speaks.

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).

from pep8speaks.

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

from pep8speaks.