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

chore: Apply minor code quality revisions #1325

Merged
merged 3 commits into from
Feb 19, 2021
Merged

chore: Apply minor code quality revisions #1325

merged 3 commits into from
Feb 19, 2021

Conversation

akshgpt7
Copy link
Contributor

@akshgpt7 akshgpt7 commented Feb 17, 2021

Description

I ran DeepSource analysis on my fork of the repo, and found some interesting code quality issues that can improve the general code quality here.
This PR fixes a few of the issues detected.

Summary of changes:

  • Refactor unnecessary use of elif/else after return.
  • Simplify if statements to use bool().
  • Refactor iterating over dictionary instead of using dict.keys().
  • Refactor unnecessary dict() call.

Checklist Before Requesting Reviewer

  • Tests are passing
  • "WIP" removed from the title of the pull request
  • Selected an Assignee for the PR to be responsible for the log summary

Before Merging

For the PR Assignees:

  • Summarize commit messages into a comprehensive review of the PR
* Apply minor code changes
   - Use raise error as default behavior to avoid if-else evaluation
   - Use evaluation of bool() over trivial if statements
   - Drop use of keys on dict iteration

@akshgpt7 akshgpt7 changed the title Refactor code quality issues chore: Refactor code quality issues Feb 17, 2021
@codecov
Copy link

codecov bot commented Feb 18, 2021

Codecov Report

Merging #1325 (a804557) into master (f1d1252) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1325   +/-   ##
=======================================
  Coverage   97.50%   97.50%           
=======================================
  Files          63       63           
  Lines        3760     3760           
  Branches      538      538           
=======================================
  Hits         3666     3666           
  Misses         55       55           
  Partials       39       39           
Flag Coverage Δ
unittests 97.50% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/pyhf/modifiers/__init__.py 86.44% <100.00%> (ø)
src/pyhf/pdf.py 96.40% <100.00%> (ø)
src/pyhf/readxml.py 94.33% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f1d1252...a804557. Read the comment docs.

@akshgpt7
Copy link
Contributor Author

@matthewfeickert The test seems to be failing for macOS when it tries to install tensorflow. Can you help me know if this is a fixable error?

@matthewfeickert
Copy link
Member

Thanks for your PR @akshgpt7! We'll take a look at this later today and get it reviewed, which should be easy enough. We're probably not interested in picking up additional config files or dependencies at the moment though, so right off the bat can we ask you to remove .deepsource.toml?

The test seems to be failing for macOS ...

The VMs for GHA sometimes can be finicky for Mac. I restarted the CI.

@akshgpt7
Copy link
Contributor Author

@matthewfeickert removed the .deepsource.toml file as requested.

Copy link
Member

@matthewfeickert matthewfeickert left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. Though this PR doesn't really deal with code quality issues, but rather is just applying changes from the scan blindly. That's fine, but some of them don't really justify the noise they would add.

If you take out the ones that I've flagged I think we'd accept this.

src/pyhf/optimize/__init__.py Outdated Show resolved Hide resolved
src/pyhf/tensor/__init__.py Outdated Show resolved Hide resolved
tests/test_export.py Outdated Show resolved Hide resolved
@matthewfeickert matthewfeickert added the chore Other changes that don't modify src or test files label Feb 19, 2021
Copy link
Member

@matthewfeickert matthewfeickert left a comment

Choose a reason for hiding this comment

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

Thanks. LGTM 👍

@matthewfeickert matthewfeickert changed the title chore: Refactor code quality issues chore: Apply minor code quality revisions Feb 19, 2021
@matthewfeickert matthewfeickert merged commit efc0ae3 into scikit-hep:master Feb 19, 2021
@akshgpt7 akshgpt7 deleted the chore-refactor-issues branch February 20, 2021 00:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Other changes that don't modify src or test files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants