Continue src/black refactoring.

[hadialqattan]

The entire Black source code has written in src/black/__init__.py which leads to poor maintainability!

[hadialqattan]

In other words, it's not easy to start contributing to black, because you can't work only with the bug related part!

[hugovk]

There was a plan to refactor black.py into a black/ Python package at some point: #1350.

At least the first bit has been done, to refactor into a src/ directory: #1376.

[ichard26]

Perhaps #1350 should be reopened then?

edit: ideally I would rename this issue to finish the refactor but I don't have the required permissions to do that so reopening/creating a new issue is easier.

[hadialqattan]

Would you like to rename it?

[hadialqattan]

Could anyone put some useful labels like design, discussion, enhancement, and stable, please?

[felix-hilden]

Hello, I'd like to ask about the status of this issue and the state of the development in general.

• I agree that having such a code base as a single file is suboptimal. It seems that at least ambv agreed in Refactor black.py into a black/ python package #1350, and as a result a plan was created, and Black was refactored under src/black/... The plan seems to have been executed fully, as it has no content beyond the basic directory structure. Has there been any concrete discussion about splitting the main source further down? Would now be a time to do start planning it?
• Existing PRs will surely have to be modified, if such a refactoring is done. What do the maintainers think about the priority of existing PRs, however stale they may be?
• What would the refactored structure look like? Do you have any ready ideas or requirements for the split?

I'm interested in contributing, because I feel like having a better structured code base could expedite all other development efforts as well. So I'd like to share my initial thoughts, though I recognise that I have no authority here.

• I see some groups of definitions that could be made into modules, like the ones involving toml and source finding, functions used by normalize_fmt_off or normalize_invisible_parens
• It seems that there are many constant operators, symbols and priorities that are either used in many places like STANDALONE_COMMENT, or in a single function like the various priorities in is_split_before_delimiter. They could be grouped into a single file, or the files where the functions that use them will end up.
• There are a couple of functions like normalize_numeric_literal (also to the first point) that have been broken down, whose components are only used in that function. They could be separated into submodules, perhaps simultaneously looking for similar functionality like fix_docstring that is also used by LineGenerator.
• There are bottom-level structures such as Feature, TargetVersion, Mode and WriteBack that could similarly be pulled away.
• Exceptions?

In general I think there are two ways of doing this:

• Having modules for all types of things, one for exceptions, constants, parsing, ... Everything cleanly separated, but lots of jumping around and importing small parts of modules.
• Defining everything where they are needed, e.g. defining STRING_PRIORITY near is_split_before_delimiter but DEFAULT_EXCLUDES near main and STATEMENT near LineGenerator. Less jumping around the source, but finding e.g. all constant definitions would be more cumbersome.

What do you think?

[ichard26]

[...] and as a result a plan was created, and Black was refactored under src/black/... The plan seems to have been executed fully, as it has no content beyond the basic directory structure. Has there been any concrete discussion about splitting the main source further down? Would now be a time to do start planning it?

The time to do this was yesterday, but no one on the core team has found enough time to come up with a plan and then discuss with the rest of the team or Black development community.

Existing PRs will surely have to be modified, if such a refactoring is done. What do the maintainers think about the priority of existing PRs, however stale they may be?

We are well aware of our PR backlog. It's something the active members (me, Jelle, and Cooper) are working on, but even us are very time limited. We are making progress, it's quite slow, but we are heading closer to 0 PRs.

What would the refactored structure look like? Do you have any ready ideas or requirements for the split?

I'm pretty sure the core team has a few ideas, but we basically never discussed them enough to come up with a concrete plan. One requirement I personally know of is that mypyc must be still usable after the refactor.

I'm interested in contributing, because I feel like having a better structured code base could expedite all other development efforts as well. So I'd like to share my initial thoughts, though I recognize that I have no authority here.

Yeah I recognize that contributing to Black is way harder than it should be. It's something we're working on, but as mentioned, no one has time. I'm personally working on rewriting the documentation (especially developer docs!) all while trying to handle PR review and issue triage, which to be honest, is a bit difficult.

In all, thanks for your ideas and voicing your thoughts. I think they'll come useful when we eventually get to this some day. I understand how difficult is it when efforts from rest of the Black development community are not given the attention they need.

[cooperlees]

I agree with either refactor designs as they are both better than where we are today. I’d like to call on other core contributors to help make a decision. I feel we should start a gdoc (or something better) to get a plan of attack and design fully our desired state then cut it up into chunks we should expect in separate PRs so multiple people can work on this.

I’d also like to suggest for each module we make an equivalent test suite is made. I expect some common / shared test libraries to be needed due to this. I’ve always been a fan of group tests to the module they test. Once again I’m all up for ideas here.

@JelleZijlstra + @ichard26 - Since you’re both most active should we start a design doc and nut this out and get some context from our users / potential contributors?

[cooperlees]

I also think a major design in this refactor should take into account that blib2to3 (thus lib2to3) will need to be replaced with a new parser in the not to distant future as it’s not going to evolve with future syntax changes for python.

[felix-hilden]

Thanks for the quick responses!

Reading between the lines a bit, I gather that PRs are a priority to you - the open ones being actually considered for merges, and that you'd like to get the count close to zero before any drastic maneuvers in the refactoring department. I also fully appreciate that you have other things to do! Better to take some time off than to be another story in the books of burnt-out OSS maintainers. I'm in the fortunate position to have both time and energy at least for a while, so I can't really fault those that don't. Improving documentation is awesome too!

I fully second having test suites for each module separately, with shared common components. To the two competing designs: I also think both are valid. I think I prefer the cleanly separated version, but I haven't worked with code bases quite as long as this, not to speak of the largest projects. Perhaps it would be best then to have some sort of open document (at least open for maintainers) to start gathering ideas over some time, while the repo advances in other ways. Would you like this issue to be the place for non-maintainers to express their views, or a separate doc?

I'd like to also share something that helped me in finding these potential refactor targets. I'm not aware of the roles of maintainers, and whether everybody is intimately familiar with the code or not. Regardless, it might help and save a bit of time at least. I wrote a small utility to visualise source code structure. I did it because of Black and my desire to contribute to it, so I included that visualisation on the RTD page as well. It's a few commits outdated now, but not too badly. Please view the image along with the legend for explanations. A small disclaimer for the beta product, but most of the ideas I presented are rather obvious examining the visualisation.

To conclude, if you don't have the time and you want to trust a stranger, I'd be happy to start formulating a plan with your guidance. Otherwise, I'm content with providing my two cents and observing where this is taken, or anything in between!

[ichard26]

To conclude, if you don't have the time and you want to trust a stranger, I'd be happy to start formulating a plan with your guidance. Otherwise, I'm content with providing my two cents and observing where this is taken, or anything in between!

Personally I'm happy to trust you, just please be warned we won't be fast with feedback. Thanks mate!

[felix-hilden]

Much appreciated! I don't mind at all. Let's do this at your pace.

I think we shall do the following:

• Set up a doc for the plan (here)
• Outline an overall strategy
• Approve of the strategy after discussions
• Wait for the current PR count to drop or the maintainers to give a green light regardless
• Gather concrete refactor ideas and start shooting reasonably-sized PRs

The doc has comment access enabled for everyone. I'll make sure to address them and transfer any comments from here as well. The reason I thought it would be better to concretize the plan only after your green light is to avoid unnecessary work with the changing source. Thoughts?