inspera / blackbricks

Black for Databricks notebooks
MIT License
44 stars 9 forks source link

Add `isort` to be executed by blackbrick #44

Closed PeterFogh closed 1 year ago

PeterFogh commented 1 year ago

Hi again, here is another feature request 😃

I think blackbrick can benefit from also executing isort for sorting Python imports to a best practice.

isort can be executed in Python on a string of code, e.g. from a notebook cell by using isort.code("import b\nimport a\n").

What is the opinion on adding more formatters than just black and sqlparser?

bsamseth commented 1 year ago

You're not alone: #26. This was discussed a while ago, but somewhat died out as I couldn't find the time and motivation to do it. I personally don't have a particular use for it, as I rarely use blackbricks on remote notebooks, and isort runs (mostly) without issue on notebook just like any other .py file.

The motivation for blackbricks to begin with was to ensure that you could run black on notebooks, as black wouldn't handle whitespace perfectly with respect to Databricks notebook format. isort on the other hand, more or less just does the right thing out of the box, and hence doesn't need any special treatment. This is mostly true, with some caveats around what to do with imports that aren't placed in the first code cell.

If it were to be added, the discussion in #26 should come to some sort of conclusion. The question is if isort is worthwhile to special case and bake in directly, or if isort is merely a single example of a more general need for various tools to "hook" into blackbricks. I'm not sure which way I'm personally leaning, and would be happy to hear arguments for either side.

TL;DR: I don't think isort needs to be bundled with blackbricks, and can instead be used as a standalone tool directly on notebooks.

stale[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.