Skip to content

Fix minor code quality issues #3626

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

Merged
merged 2 commits into from
Apr 2, 2020
Merged

Fix minor code quality issues #3626

merged 2 commits into from
Apr 2, 2020

Conversation

sauravsrijan
Copy link
Contributor

Changes:

  • Use _ for unused variables
  • Use """ for docstrings
  • Add .deepsource.toml file to run continuous static analysis on the repository with DeepSource

This PR fixes some minor style issues and set up continuous static analysis on the repo with DeepSource. Upon enabling DeepSource, quality and security analysis will be run on every PR to detect 500+ types of problems in the changes — including bug risks, anti-patterns, security vulnerabilities, type-check issues, etc.
DeepSource is free to use for open-source projects, and is used by teams at NASA, Uber, Slack among many others, and open-source projects like ThoughtWorks/Gauge, Masonite Framework, etc.
To enable DeepSource analysis after merging this PR, please follow these steps:

  • Sign up on DeepSource with your GitHub account and grant access to this repo.
  • 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!

Changes:

- Use `_` for unused variables
- Use `"""` for docstrings
- Add `.deepsource.toml` file to run continuous static analysis on repository with DeepSource
.deepsource.toml Outdated

[analyzers.meta]
runtime_version = "3.x.x"
type_checker = "mypy"
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this redundant with the Azure CI job we already have?

@crusaderky
Copy link
Contributor

If you merge from master you should get all green on the upstream-dev test now

@crusaderky
Copy link
Contributor

Ok, I've played with it a bit and it's a hard -1 from me.

  • I could not find a way to run it locally (correct me if I'm wrong); meaning all PRs will need to go through a million of trial-and-error commits and pushes
  • Very heavily overlapping with what we already have (flake8, black, isort, mypy)
  • Will trigger on issues that are explictly (made) ok for the other linter tools

@sauravsrijan best of luck with your project, even if we won't be the first library to take this up
@max-sixty
Copy link
Collaborator

max-sixty commented Apr 2, 2020

Merging these as helpful style changes (thank you @sauravsrijan ), even if we're not implementing DeepSource at the moment. We're open to reviewing in the future.

@max-sixty max-sixty merged commit 1ed4f4d into pydata:master Apr 2, 2020
dcherian added a commit to dcherian/xarray that referenced this pull request Apr 5, 2020
* master:
  Use divergent colormap if lowest and highest level span 0 (pydata#3913)
  Bugfix for plotting transposed 2d coords (pydata#3934)
  Allow plotting bool data (pydata#3766)
  facetgrid: fix case when vmin == vmax (pydata#3916)
  add a CI that tests xarray with all optional dependencies but dask (pydata#3919)
  Add missing_dims argument allowing isel() to ignore missing dimensions (pydata#3923)
  Only fail if a specific warning occurs (pydata#3930)
  Fix minor code quality issues (pydata#3626)
  Fix for stack+groupby+apply w/ non-increasing coord (pydata#3906)
  reactivate the macos CI (pydata#3920)
  add pint to the output of show_versions() (pydata#3918)
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.

4 participants