Code Quality #664
Closed
Code Quality #664
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fix `test_nonnumeric_magnitudes`
Should be 'if cond is None:'
Should be 'not in'
Should be 'is not'
Refactor min -> minimum
Refactor max -> maximum
Bare except also catches unexpected events like memory errors, interrupts, system exit, and so on. Prefer the specific exception or `except Exception:` for general exceptions. If you're sure what you're doing, be explicit and write `except BaseException:`.
|
What is the relation of this and #661 ? |
|
Doh it merged my two branches when I pushed them to my local fork after creating the PR. |
|
Thanks for the PR. It looks good to me except for those two comments which I am not sure. |
bors bot
added a commit
that referenced
this pull request
Sep 6, 2018
684: Add pandas support r=hgrecco a=znicholls This pull request adds pandas support to pint (hence is related to #645, #401 and pandas-dev/pandas#10349). An example can be seen in `example-notebooks/basic-example.ipynb`. It's a little bit hacksih, feedback would be greatly appreciated by me and @andrewgsavage. One obvious example is that we have to run all the interface tests with `pytest` to fit with `pandas` test suite, which introduces a dependency for the CI and currently gives us this awkward testing setup (see the alterations we had to make to `testsuite`). This also means that our code coverage tests are fiddly too. If you'd like us to squash the commits, that can be done. If pint has a linter, it would be good to run that over this pull request too as we're a little bit all over the place re style. Things to discuss: - [x] general feedback and changes - [x] test setup, especially need for pytest for pandas tests and hackish way to get around automatic discovery - [x] squashing/rebasing - [x] linting/other code style (related to #664 and #628: we're happy with whatever, I've found using an automatic linter e.g. black and/or flake8 has made things much simpler in other projects) - [x] including notebooks in the repo (if we want to, I'm happy to put them under CI so we can make sure they run) - [x] setting up the docs correctly Co-authored-by: Zebedee Nicholls <[email protected]> Co-authored-by: andrewgsavage <[email protected]>
|
This PR looks pretty straightforward and useful, is there any reason for postponing the merge? |
|
I have embedded all these changes into #931, please close |
bors bot
added a commit
that referenced
this pull request
Dec 16, 2019
931: black, flake8, and isort (part 2) r=hgrecco a=crusaderky - Configure flake8 and isort - Resolve all flake8 errors - Assorted code quality tweaks (embeds #664) - Out of scope: CI integration and documentation (will be done in a successive PR) Co-authored-by: Guido Imperiale <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes the following: