Skip to content

Reformat continuous.py #4051

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 8 commits into from
Sep 7, 2020

Conversation

AlexAndorra
Copy link
Contributor

@AlexAndorra AlexAndorra commented Aug 14, 2020

As discussed in #4049, this PR cleans up continuous.py for better readability of the code:

  1. Reorders import statements according to PEP8
  2. Blackifies the whole file

In addition, @junpenglao mentioned repeating bits that could be refactored. Do you have specific places in mind Junpeng?
I can make the changes or you can push directly on this PR -- whatever you prefer 🙂

  • No breaking changes -- only formatting changes
  • No need to mention in RELEASE-NOTES.md

@codecov
Copy link

codecov bot commented Aug 14, 2020

Codecov Report

Merging #4051 into master will increase coverage by 0.00%.
The diff coverage is 92.11%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4051   +/-   ##
=======================================
  Coverage   88.47%   88.48%           
=======================================
  Files          88       88           
  Lines       13860    13865    +5     
=======================================
+ Hits        12263    12268    +5     
  Misses       1597     1597           
Impacted Files Coverage Δ
pymc3/distributions/continuous.py 92.82% <92.11%> (+0.04%) ⬆️

@junpenglao
Copy link
Member

Some of the Black formatting I am not sure it is making the code better or worse 😂

@AlexAndorra
Copy link
Contributor Author

Ha ha yeah, I see what you mean! I think this is always a trade-off though, as it also makes some parts of the code much clearer -- so I hope on balance it's better 😅
I just added a pyproject.toml file at the root, to set the vertical limit at 100 instead of 88 -- as we do on ArviZ. Should help with the trade-off 😉

Thanks a lot for all these reformatting proposals, I'll look at that ASAP!

@AlexAndorra
Copy link
Contributor Author

AlexAndorra commented Aug 28, 2020

Turns out the "ASAP" was a bit long this time around, but I added your suggested reformattings @junpenglao 😉
Tell me what you think, and let's see if tests pass!

@AlexAndorra AlexAndorra requested a review from junpenglao August 30, 2020 22:24
@AlexAndorra
Copy link
Contributor Author

Fixing merge conflicts then self-merging if nobody has strong feelings against this reformatting -- to avoid further merge conflicts

@canyon289
Copy link
Member

canyon289 commented Sep 7, 2020

Black is making some of the lines worse to read as @junpenglao mentioned, but knowing how black doesn't care about our feelings Im not sure what you can do about it

@AlexAndorra AlexAndorra merged commit 35a4a46 into pymc-devs:master Sep 7, 2020
@AlexAndorra AlexAndorra deleted the reformat-continuous-py branch September 7, 2020 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants