Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

pep8speaks #2462

Merged
merged 6 commits into from
Oct 7, 2018
Merged

pep8speaks #2462

merged 6 commits into from
Oct 7, 2018

Conversation

fujiisoup
Copy link
Member

I installed pep8speaks as suggested in #2428.
It looks they do not need a yml file, but it may be safer to add this (just renamed from .stickler.yml)

@fujiisoup
Copy link
Member Author

@pep8speaks suggest the diff

@pep8speaks
Copy link

pep8speaks commented Oct 4, 2018 •
edited
Loading

Hello @fujiisoup! Thanks for updating the PR.

Comment last updated on October 07, 2018 at 19:32 Hours UTC

@fujiisoup
Copy link
Member Author

@pep8speaks suggest the diff

@fujiisoup fujiisoup mentioned this pull request Oct 4, 2018
Closed
@fmaussion
Copy link
Member

Thanks! I just switched off stickler which is still broken anyway.

@fujiisoup fujiisoup closed this Oct 4, 2018
@fmaussion
Copy link
Member

I think we need a set-up file to set diff_only on PR. See the pandas pep8speaks template: https://github.com/pandas-dev/pandas/blob/master/.pep8speaks.yml

@fmaussion fmaussion reopened this Oct 4, 2018
@max-sixty
Copy link
Collaborator

Ref #2467 (comment)

@max-sixty
Copy link
Collaborator

I think we need a set-up file to set diff_only on PR

Why do we need diff_only? The repo passes flake8 as a whole on my local

@fmaussion
Copy link
Member

Why do we need diff_only

I was worried about all the unrelated pep8 errors on this PR: #2463

Although I'm not sure where they come from.

@max-sixty
Copy link
Collaborator

Good point.

That's odd - I don't get those on #2467, or locally.

Is there an issue around ignoring # noqa?

@fmaussion
Copy link
Member

I also have the impression that noqa are ignored by pep8speask, although I haven't seen any related issue on their repo. Probably another reason for a diff_only option for now.

@max-sixty
Copy link
Collaborator

OK. Though if people make changes to # noqa lines, we'll have false positives.

Weird that @stickler-ci broke. We're still using it at pandas-gbq and it's working well.

@fmaussion
Copy link
Member

Weird that @stickler-ci broke. We're still using it at pandas-gbq and it's working well.

I could try to reactivate it now that I've deactivated it. Still suspicious that clicking @stickler-ci gives a 404 error.

I like pep8speaks for it's less verbose messaging (all in one message that gets updated after each new commit) and for the fact that pandas is also using it, but I have no strong opinion about it.

If we use pep8speaks we should use the same config as pandas though, and we should ask them about #noqa : done here: pep8speaks-org/pep8speaks#96

@fujiisoup
Copy link
Member Author

I just updated .pep8speaks.yml that is taken from pandas-dev/pandas.
If there are no further comments, I would merge this. Let's see how it works with this configuration.

@shoyer shoyer merged commit cf1e6c7 into pydata:master Oct 7, 2018
@shoyer
Copy link
Member

shoyer commented Oct 7, 2018

👍 Let's give this is a try

dcherian pushed a commit to maahn/xarray that referenced this pull request Oct 10, 2018
Merge branch 'master' into maahn-groupy_plot2
826df44 
* master: (51 commits)
  xarray.backends refactor (pydata#2261)
  Fix indexing error for data loaded with open_rasterio (pydata#2456)
  Properly support user-provided norm. (pydata#2443)
  pep8speaks (pydata#2462)
  isort (pydata#2469)
  tests shoudn't need to pass for a PR (pydata#2471)
  Replace the last of unittest with pytest (pydata#2467)
  Add python_requires to setup.py (pydata#2465)
  Update whats-new.rst (pydata#2466)
  Clean up _parse_array_of_cftime_strings (pydata#2464)
  plot.contour: Don't make cmap if colors is a single color. (pydata#2453)
  np.AxisError was added in numpy 1.13 (pydata#2455)
  Add CFTimeIndex.shift (pydata#2431)
  Fix FutureWarning in CFTimeIndex.date_type (pydata#2448)
  fix:2445 (pydata#2446)
  Enable use of cftime.datetime coordinates with differentiate and interp (pydata#2434)
  restore ddof support in std (pydata#2447)
  Future warning for default reduction dimension of groupby (pydata#2366)
  Remove incorrect statement about "drop" in the text docs (pydata#2439)
  Use profile mechanism, not no-op mutation (pydata#2442)
  ...
dcherian pushed a commit to dcherian/xarray that referenced this pull request Oct 10, 2018
Merge branch 'master' into 2d-coord
2a2ab82 
* master: (21 commits)
  xarray.backends refactor (pydata#2261)
  Fix indexing error for data loaded with open_rasterio (pydata#2456)
  Properly support user-provided norm. (pydata#2443)
  pep8speaks (pydata#2462)
  isort (pydata#2469)
  tests shoudn't need to pass for a PR (pydata#2471)
  Replace the last of unittest with pytest (pydata#2467)
  Add python_requires to setup.py (pydata#2465)
  Update whats-new.rst (pydata#2466)
  Clean up _parse_array_of_cftime_strings (pydata#2464)
  plot.contour: Don't make cmap if colors is a single color. (pydata#2453)
  np.AxisError was added in numpy 1.13 (pydata#2455)
  Add CFTimeIndex.shift (pydata#2431)
  Fix FutureWarning in CFTimeIndex.date_type (pydata#2448)
  fix:2445 (pydata#2446)
  Enable use of cftime.datetime coordinates with differentiate and interp (pydata#2434)
  restore ddof support in std (pydata#2447)
  Future warning for default reduction dimension of groupby (pydata#2366)
  Remove incorrect statement about "drop" in the text docs (pydata#2439)
  Use profile mechanism, not no-op mutation (pydata#2442)
  ...
@OrkoHunter OrkoHunter mentioned this pull request Mar 12, 2019
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@fujiisoup @pep8speaks @fmaussion @max-sixty @shoyer