Skip to content

Update APE20 for Black formatting not just the Black tool - #90

Merged
eteq merged 1 commit into
astropy:mainfrom
taldcroft:ape20-update-allow-other-auto-formatters
Jan 22, 2024
Merged

eteq merged 1 commit into
astropy:mainfrom
taldcroft:ape20-update-allow-other-auto-formatters

Conversation

@taldcroft

Copy link
Copy Markdown
Member

Since APE-20 was accepted in 2022, another auto-formatting tool has become available that also implements the default Black format and has gained popularity. This development highlights that APE-20 was written with a somewhat short-sighted perspective -- it focuses on using the Black tool instead of adopting Black formatting using whatever Black-compatible auto-formatter is appropriate.

Changing the entire PR to reflect this change is not that helpful, but instead this PR adds a few words in the abstract to reflect this broader perspective. Adoption of this PR is technically a lien on changing the astropy Black-format CI check to another tool.

@pllim

pllim commented Dec 19, 2023 •

Copy link
Copy Markdown
Member

Thanks! I will put this on agenda for dev telecon.

xref astropy/astropy#15757

@mhvk mhvk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Makes sense to me, thanks for the update.

@WilliamJamieson WilliamJamieson left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I agree with this change in principle.

@pllim

pllim commented Jan 4, 2024

Copy link
Copy Markdown
Member

So far no one objects, so I added this to CoCo agenda for 2024-01-22 when we next meet. Thanks for your patience!

@astrofrog

Copy link
Copy Markdown
Member

If uncontroversial this could be put to the full voting members list for a vote instead of waiting for the coordination meeting which might only have a fraction of voting members.

@dhomeier

dhomeier commented Jan 4, 2024

Copy link
Copy Markdown
Contributor

Fully agree with making this tool-independent. Perhaps the heading should also be changed?

Formatting Code in Black Style

And since the modified text now becomes a little bit more specific about what is the Black code style, it would be good to note that its exact incarnation to be followed is decided per case, sort of.
In particular since Ruff has some history of pulling in whatever formatting or linting rules can be found as (optionally) available ruleset, I am wondering if its "standard" format will always stay in sync with Black itself (but even when we first adopted it, I think Black itself had somewhat different rulesets in stable and developer versions).

@pllim

pllim commented Jan 4, 2024

Copy link
Copy Markdown
Member

put to the full voting members list for a vote

I guess https://github.com/astropy/astropy-APEs/blob/main/APE0.rst#responsibilities-and-rights does not rule that possibility out, but isn't that slower? Someone has to set up the heliovotebot (or whatever it is called) and then go through the 2 weeks voting period, get the tallies, and so on?

@neutrinoceros

Copy link
Copy Markdown

@dhomeier

I am wondering if its "standard" format will always stay in sync with Black itself

It may be too soon to tell for sure, but their documented philosophy feels like they want/have to maintain black-compatibility long term (else, they would make migration much less convenient, which would kill most of their argument). It's also worth noting that black and ruff contributors have been working hand in hand for a little while (or at least seem to be talking very regularly).

@astrofrog

Copy link
Copy Markdown
Member

I guess https://github.com/astropy/astropy-APEs/blob/main/APE0.rst#responsibilities-and-rights does not rule that possibility out, but isn't that slower? Someone has to set up the heliovotebot (or whatever it is called) and then go through the 2 weeks voting period, get the tallies, and so on?

Yeah sorry I misremembered the process and this is indeed overkill. But maybe we can even reach a consensus before the coordination meeting.

@mhvk

mhvk commented Jan 5, 2024

Copy link
Copy Markdown
Contributor

This really seems super uncontroversial - the APE really was about settling on a specific style, with surely the tool an implementation detail. I'd consensus has already become clear!

@taldcroft

Copy link
Copy Markdown
Member Author

From what I can see this has been uncontroversial and there is consensus. Jan 22 will be here before we know it, time flies! Is there a PR ready or in-work for migrating to ruff format in astropy?

@neutrinoceros

neutrinoceros commented Jan 6, 2024 •

Copy link
Copy Markdown

There isn't. Can I volunteer ?

Edit: just remembered that @eerovaher had some plans for how to do it. I don't want to step on his toes, but I'm here to help !

@eerovaher

Copy link
Copy Markdown
Member

My plan (that I mentioned at a developer telecon a couple of months ago) was to have a look at the lines in astropy that Ruff formatter would edit and to see if they could be rewritten so that both Black and Ruff formatter would be happy with them. The goal was to reduce the size of the patch that would be required to switch from Black to Ruff formatter, but also to try to improve odd-looking code if possible.

The part of the transitioning that requires the most effort to implement and to review is updating the developer documentation, and my plan has nothing to do with that.

@neutrinoceros

neutrinoceros commented Jan 8, 2024 •

Copy link
Copy Markdown

Ok, then I can do it, as I think I should read that doc anyway ! (I'll still wait for the present update to go in)

@eerovaher eerovaher left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I am in favor of astropy switching from using Black to using Ruff formatter. We are using Ruff for linting anyways.

@dhomeier dhomeier left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good now, thanks!

@eteq eteq left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Happy with this too! I think if a third one shows up we might want to re-word this to not have to update it every time a new tool comes out but this might be the last one so 🤷

@ceb8

ceb8 commented Jan 22, 2024

Copy link
Copy Markdown
Member

I also approve.

@eteq

eteq commented Jan 22, 2024

Copy link
Copy Markdown
Member

The CoCo today unanimously approved this, will merge shortly!

@eteq
eteq merged commit a29e80e into astropy:main Jan 22, 2024
@eteq

eteq commented Jan 22, 2024

Copy link
Copy Markdown
Member

Hmm, I think someone else might own the Zenodo entry, so it has not been updated yet. Trying to track down who it is...

@pllim

pllim commented Jan 22, 2024

Copy link
Copy Markdown
Member

Oh no... maybe it was me. 😅

@pllim

pllim commented Jan 24, 2024 •

Copy link
Copy Markdown
Member

OK never heard back from Erik, so if it is indeed under my own Zenodo account, I am just going to update that same record. I don't have time to mess around with this stuff.

ac97aea

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.

10 participants