Skip to content

Conversation

@bjlittle
Copy link
Member

@bjlittle bjlittle commented Sep 17, 2020

🚀 Pull Request

Description

🎬 Take two...

Encountered unexpected minor carnage in #3864, when attempting to upgrade the black version from 19.10b0 to 20.8b1, so it was actually easier to close #3864 and start again 😉

Okay, so this was interesting... you could just leave black to do it's thing, and mindlessly accept what it generates. However, a wee bit of manual intervention is required in the case where black had previously (19.10b0) injected trailing ,'s at the end of sequences et al... which is fine in itself, but when injested by 20.8b1 it reformats it in a rather hideous way e.g., this

np.logical_and(mask_invert[:-1,], diffs_along_y)

goes to

np.logical_and(
    mask_invert[
        :-1,
    ],
    diffs_along_y,
)

🤮

In response, the manual intervention involves deciding whether the previously injected , was necessary or not, particularly if it was causing icky reformatting to occur.

Also, I ended up making acceptable but slightly subtle tuples more explicit, also in order to avoid over-blackification™️ (that's now an official term btw 😉) e.g., this

(2, 0, 1),

becomes this

((2, 0, 1),)

Meh... I'm not overly offended by it, particularly when it stops this

         self.check((3,), Index[(2, 0, 1),], [(np.array([2, 0, 1]), Ellipsis)])

transmuting into this little rose bud

        self.check(
            (3,),
            Index[
                (2, 0, 1),
            ],
            [(np.array([2, 0, 1]), Ellipsis)],
        )

Consult Iris pull request check list

@bjlittle bjlittle requested a review from rcomer September 17, 2020 13:40
@trexfeathers
Copy link
Contributor

e.g., this
(2, 0, 1),

becomes this
((2, 0, 1),)

That change is generally quite helpful to a Python yearling like me ❤

@bjlittle
Copy link
Member Author

@trexfeathers I suppose this requires a whatsnew entry, right?

If so, would you prefer me to tack it along with this PR, or raise it separately?

@trexfeathers
Copy link
Contributor

If so, would you prefer me to tack it along with this PR, or raise it separately?

I hope this will be a quick one, so just include it here. Thanks!

Copy link
Contributor

@trexfeathers trexfeathers left a comment

Choose a reason for hiding this comment

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

Great, thanks for persevering @bjlittle 🖌️ ⚫

@trexfeathers trexfeathers merged commit 8e4e0a4 into SciTools:master Sep 17, 2020
@bjlittle
Copy link
Member Author

bjlittle commented Sep 17, 2020

@trexfeathers Awesome, thanks! 👍

@rcomer
Copy link
Member

rcomer commented Sep 17, 2020

Wow, just wow. 🤯

tkknight added a commit to tkknight/iris that referenced this pull request Sep 20, 2020
* master:
  Whatsnew for effects on aux factories of units defaulting to 'unknown'. (SciTools#3870)
  Whatsnew entry for SciTools#3867. (SciTools#3868)
  Developer guide overhaul (SciTools#3852)
  Update CF standard name table to v75 (SciTools#3867)
  Link to new classes and methods in the Ancillary variables whatsnew. (SciTools#3865)
  update black version (SciTools#3866)
  Fix whatsnew api links. (SciTools#3856)
  Add additional pre-commit hooks (SciTools#3862)
  update pre-commit flake8 version (SciTools#3863)
  whatsnew - update announcement (SciTools#3861)
  whatsnew - remove contents directive (SciTools#3859)
  whatsnew - links and versions (SciTools#3853)
  Replace deprecated IndexFormatter (SciTools#3857)
  whatsnew for SciTools#3681 (SciTools#3858)
  Whatsnew entry for SciTools#3846. (SciTools#3855)
  Image tests: set agg backend after rcdefaults (SciTools#3846)
  whatnew - announcements (SciTools#3850)
@bjlittle bjlittle deleted the update-black-version branch October 1, 2020 12:45
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