Skip to content
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

Black should always explode multiline collection literals #152

Closed
dstufft opened this issue Apr 20, 2018 · 7 comments
Closed

Black should always explode multiline collection literals #152

dstufft opened this issue Apr 20, 2018 · 7 comments
Labels
T: style What do we want Blackened code to look like?

Comments

@dstufft
Copy link

dstufft commented Apr 20, 2018

Operating system: macOS
Python version: 3.6.1
Black version: 18.4a2
Does also happen on master: Yes

After running black, I've gotten code formatted like:

class Classifier(db.ModelBase):

    __tablename__ = "trove_classifiers"
    __table_args__ = (
        Index("trove_class_class_idx", "classifier"), Index("trove_class_id_idx", "id")
    )

    __repr__ = make_repr("classifier")

    id = Column(Integer, primary_key=True, nullable=False)
    classifier = Column(Text, unique=True)
    deprecated = Column(Boolean, nullable=False, server_default=sql.false())
    l2 = Column(Integer)
    l3 = Column(Integer)
    l4 = Column(Integer)
    l5 = Column(Integer)

While I understand the intention is for a sequence that can fit on one line to be on one line, this list here obviously can't fit on one line (it's been broken over 3 lines!) but it can only fit onto one line when you remove part of the syntax that makes it a list/tuple/whatever.

I think that it would be easier to read if the detection of a multi-line sequence was more aggressive, either it should all fit onto one line, or it should be formatted like:

class Classifier(db.ModelBase):

    __tablename__ = "trove_classifiers"
    __table_args__ = (
        Index("trove_class_class_idx", "classifier"),
        Index("trove_class_id_idx", "id"),
    )

    __repr__ = make_repr("classifier")

    id = Column(Integer, primary_key=True, nullable=False)
    classifier = Column(Text, unique=True)
    deprecated = Column(Boolean, nullable=False, server_default=sql.false())
    l2 = Column(Integer)
    l3 = Column(Integer)
    l4 = Column(Integer)
    l5 = Column(Integer)
@ambv ambv added the T: style What do we want Blackened code to look like? label Apr 20, 2018
@carljm
Copy link
Collaborator

carljm commented Apr 20, 2018

TBH I am highly sympathetic to this argument.

@ambv
Copy link
Collaborator

ambv commented Apr 20, 2018

This turns out to be a common complaint.

The current behavior is pretty core to how Black formats all bracket content. This includes collection literals (tuples, lists, sets, dicts), function calls and function signatures.

Since we probably already have to bail on this behavior because of isort, it makes sense to evolve Black such that:

  • function calls and function signatures have the immediate step;
  • collection literals and from_imports don't.

What do you think? (cc @hynek)

I lament that it will complicate my code but that's my problem.

@dstufft
Copy link
Author

dstufft commented Apr 20, 2018

I lament that it will complicate my code but that's my problem.

I'm just here to ruin your day with complexity ;)

@ambv ambv changed the title Multi-line sequence detection not aggressive enough Black should always explode multiline collection literals and multiline imports Apr 20, 2018
@hynek
Copy link
Contributor

hynek commented Apr 21, 2018

Well, as you know, I’m a big fan of exploding everything. :) I’m very bad at reading long lines (especially with dicts).

@ambv
Copy link
Collaborator

ambv commented Apr 24, 2018

The import part of this is now done. For collection literals I have to figure out a consistent way of doing it.

ambv added a commit that referenced this issue Apr 24, 2018
Fixes #127

Partially addresses #152
@ambv ambv changed the title Black should always explode multiline collection literals and multiline imports Black should always explode multiline collection literals Apr 26, 2018
@dstufft
Copy link
Author

dstufft commented May 16, 2018

Just to verify, this covers:

env = Environment(
    loader=FileSystemLoader(dir_name),
    extensions=[
-            "jinja2.ext.i18n",
-            "warehouse.utils.html.ClientSideIncludeExtension",
+            "jinja2.ext.i18n", "warehouse.utils.html.ClientSideIncludeExtension"
    ],
    cache_size=0,
)

Right?

@ambv
Copy link
Collaborator

ambv commented May 17, 2018

Yup.

@ambv ambv closed this as completed in dd4477b May 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T: style What do we want Blackened code to look like?
Projects
None yet
Development

No branches or pull requests

4 participants