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

yapf formats long call chains with weird spacing #617

Open
qv-pat opened this issue Sep 13, 2018 · 5 comments
Open

yapf formats long call chains with weird spacing #617

qv-pat opened this issue Sep 13, 2018 · 5 comments

Comments

@qv-pat
Copy link

qv-pat commented Sep 13, 2018

Take the following lengthy call chain

def some_function(session):
    result = (session.query(models.Employee.id).filter(models.Employee.business_id == unique_id).filter(models.Employee.age > 25).order_by(models.Employee.id.asc()).all())

I'm would love an output that looked something like this:

def some_function(session):
    result = (
        session.query(models.Employee.id)
        .filter(models.Employee.business_id == unique_id)
        .filter(models.Employee.age > 25)
        .order_by(models.Employee.id.asc())
        .all()
    )

But instead yapf is doing this:

def some_function(session):
    result = (
        session.query(models.Employee.id
                      ).filter(models.Employee.business_id == unique_id
                               ).filter(models.Employee.age > 25
                                        ).order_by(models.Employee.id.asc()
                                                   ).all()
    )

I've tried everything I can think of

  • CONTINUATION_ALIGN_STYLE
  • SPLIT_BEFORE_DOT
  • SPLIT_BEFORE_EXPRESSION_AFTER_OPENING_PAREN
  • Adjusting the SPLIT_PENALTY_AFTER_OPENING_BRACKET value

It wouldn't be so bad if it didn't change my formatting from that second code block to the third. Right now the only thing I can think of is to either wrap the blocks with #yapf: disable tags or to use back slashes, both of which I'm pretty against.

yapf==0.24.0

@joshburkart
Copy link

I tend to work around this by putting in fake comments:

a = (  #
    session.query(...)  #
    .filter(...)  #
    .filter(...)  #
    ...
)

@carlthome
Copy link

👍

In TensorFlow's ETL API tf.data a chain of method calls is very common. It would be great to see YAPF give a nice default behavior for such fluent interfaces.

Example

Before YAPF

def create_dataset(metadata: pd.DataFrame) -> tf.data.Dataset:
    dataset = (
        tf.data.Dataset
        .from_tensor_slices(dict(metadata))
        .map(load, 256)
        .map(collate)
        .shuffle(256)
        .batch(32, drop_remainder=True)
        .prefetch(1)
    )
    return dataset

After YAPF (yapf==0.28.0 defaults)

def create_dataset(metadata: pd.DataFrame) -> tf.data.Dataset:
    dataset = (tf.data.Dataset.from_tensor_slices(dict(metadata)).map(
        load,
        256).map(collate).shuffle(256).batch(32,
                                             drop_remainder=True).prefetch(1))
    return dataset

@sylann
Copy link

sylann commented May 14, 2020

It is possible to write chains like this to prevent yapf from changing the indentation:

def some_function(session):
    result = session.query(models.Employee.id) \
        .filter(models.Employee.business_id == unique_id) \
        .filter(models.Employee.age > 25) \
        .order_by(models.Employee.id.asc()) \
        .all()

Similar to the funny usage of empty comments above.
(Note that the space before the \ is not necessary at all, I just find it more readable.)

However, I still prefer the way the op asked in the first place, because it's easier to arrange and in some cases, you could put a call chain as an argument to another function call.

session.query(
    session.query(models.Employee.id)
    .filter(models.Employee.business_id == unique_id)
    .filter(models.Employee.age > 25)
    .order_by(models.Employee.id.asc())
    .exists()
).scalar()

In this case, the backslash trick from above wouldn't be allowed, (contrary to the empty comment trick), and yapf will reformat like this:

session.query(
    session.query(models.Employee.id).filter(models.Employee.business_id == unique_id
                                             ).filter(models.Employee.age > 25
                                                      ).order_by(models.Employee.id.asc()).exists()
).scalar()

I know black deals with this syntax "properly", but it's terribly off on many other things, so I doubt i'll ever use it.
Anyone knows if it's possible that Yapf tackles this issue? I would like to start using a formatter.

@BenC14
Copy link

BenC14 commented May 13, 2021

For reference black had this figured out in 2018 feels like something that should be brought to YAPF at least as an option. psf/black#67 (comment)

@bwendling
Copy link
Member

YAPF definitely doesn't have good support for long calling chains (as you guys have noticed). Basically, YAPF doesn't identify long calling chains well. It's kind of tricky in general (there's some really heinous code that deals with these things). The best way to deal with this in YAPF is to identify individual calls and give them a higher "split cost" that the stuff around them. It's theoretically possible, just hasn't been done...

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

No branches or pull requests

6 participants