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.

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

Fix some code quality and bug-risk issues #3999

Merged
merged 1 commit into from
Apr 24, 2020

Conversation

pnijhara
Copy link
Contributor

@pnijhara pnijhara commented Apr 23, 2020

  • Fix dangerous default argument
  • Remove unnecessary comprehension

Find the other issues found here - https://deepsource.io/gh/pnijhara/xarray/issues/?category=all
This PR also adds .deepsource.toml configuration file to run DeepSource analysis on the repo with. Upon enabling DeepSource, the analysis will run on every PR and commit to detect 560+ types of issues in the changes — including bug risks, anti-patterns, security vulnerabilities, etc.

To enable DeepSource analysis after merging this PR, please follow these steps:

  1. Signup on DeepSource with your GitHub account and grant access to this repo.
  2. Activate analysis on this repo here

You can also look at the docs for more details. Do let me know if I can be of any help!

@pep8speaks
Copy link

pep8speaks commented Apr 23, 2020

Hello @pnijhara! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-04-24 04:19:16 UTC

@pnijhara pnijhara marked this pull request as draft April 23, 2020 23:30
Copy link
Member

@shoyer shoyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pnijhara thanks for putting this together!

We would gladly accept pull requests to fix many of these issues, but they will need to be done selectively.

Scanning through the list of 1900 (!) reported issues from deepsource.io, most are only (debatable) code quality issues and not real bugs. The signal to noise ratio is not high enough to add these alerts to all of our pull requests.

xarray/coding/cftime_offsets.py Outdated Show resolved Hide resolved
@pnijhara
Copy link
Contributor Author

@shoyer Thanks for the suggestions. I have re-introduced len() over there. Seems like tests were written by taking dates as numpy array and not just simple array.

@pnijhara pnijhara marked this pull request as ready for review April 24, 2020 03:01
@pnijhara pnijhara requested a review from shoyer April 24, 2020 03:01
Comment on lines +1 to +18
version = 1

test_patterns = [
"*/tests/**",
"*/test_*.py"
]

exclude_patterns = [
"doc/**",
"ci/**"
]

[[analyzers]]
name = "python"
enabled = true

[analyzers.meta]
runtime_version = "3.x.x"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this necessary for deepsource to give useful results? I guess we can give it a try if you want to keep on making contributions using it, but I don't want to add it to CI at this time and if it doesn't get much use we'll probably eventually remove it.

Copy link
Contributor Author

@pnijhara pnijhara Apr 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I know, Deepsource gives useful results. As it is used by many other projects like https://github.com/uber/ludwig and https://github.com/slackapi/python-slackclient/
I think Pydata should also give it a try.

MANIFEST.in Outdated Show resolved Hide resolved
@pnijhara pnijhara requested a review from shoyer April 24, 2020 04:23
@shoyer shoyer merged commit 37551da into pydata:master Apr 24, 2020
@shoyer
Copy link
Member

shoyer commented Apr 24, 2020

Thanks

dcherian added a commit to dcherian/xarray that referenced this pull request May 1, 2020
* upstream/master: (39 commits)
  Pint support for DataArray (pydata#3643)
  Apply blackdoc to the documentation (pydata#4012)
  ensure Variable._repr_html_ works (pydata#3973)
  Fix handling of abbreviated units like msec (pydata#3998)
  full_like: error on non-scalar fill_value (pydata#3979)
  Fix some code quality and bug-risk issues (pydata#3999)
  DOC: add pandas.DataFrame.to_xarray (pydata#3994)
  Better chunking error messages for zarr backend (pydata#3983)
  Silence sphinx warnings (pydata#3990)
  Fix distributed tests on upstream-dev (pydata#3989)
  Add multi-dimensional extrapolation example and mention different behavior of kwargs in interp (pydata#3956)
  keep attrs in interpolate_na (pydata#3970)
  actually use preformatted text in the details summary (pydata#3978)
  facetgrid: Ensure that colormap params are only determined once. (pydata#3915)
  RasterioDeprecationWarning (pydata#3964)
  Empty line missing for DataArray.assign_coords doc (pydata#3963)
  New coords to existing dim (doc) (pydata#3958)
  implement a more threadsafe call to colorbar (pydata#3944)
  Fix wrong order of coordinate converted from pd.series with MultiIndex (pydata#3953)
  Updated list of core developers (pydata#3943)
  ...
@pnijhara pnijhara deleted the pnijhara-patch-1 branch May 5, 2020 02:21
dcherian added a commit to kmuehlbauer/xarray that referenced this pull request May 9, 2020
…k-issues

* upstream/master: (22 commits)
  support darkmode (pydata#4036)
  Use literal syntax instead of function calls to create the data structure (pydata#4038)
  Add template xarray object kwarg to map_blocks (pydata#3816)
  Transpose coords by default (pydata#3824)
  Remove broken test for Panel with to_pandas() (pydata#4028)
  Allow warning with cartopy in docs plotting build (pydata#4032)
  Support overriding existing variables in to_zarr() without appending (pydata#4029)
  chore: Remove unnecessary comprehension (pydata#4026)
  fix to_netcdf docstring typo (pydata#4021)
  Pint support for DataArray (pydata#3643)
  Apply blackdoc to the documentation (pydata#4012)
  ensure Variable._repr_html_ works (pydata#3973)
  Fix handling of abbreviated units like msec (pydata#3998)
  full_like: error on non-scalar fill_value (pydata#3979)
  Fix some code quality and bug-risk issues (pydata#3999)
  DOC: add pandas.DataFrame.to_xarray (pydata#3994)
  Better chunking error messages for zarr backend (pydata#3983)
  Silence sphinx warnings (pydata#3990)
  Fix distributed tests on upstream-dev (pydata#3989)
  Add multi-dimensional extrapolation example and mention different behavior of kwargs in interp (pydata#3956)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants