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

New file to stdout #164

Closed
AckslD opened this issue Jul 15, 2021 · 15 comments
Closed

New file to stdout #164

AckslD opened this issue Jul 15, 2021 · 15 comments

Comments

@AckslD
Copy link

AckslD commented Jul 15, 2021

Is it possible to pipe the new file to stdout? Would be nice so that one could use filter in vim :)

@AckslD
Copy link
Author

AckslD commented Jul 15, 2021

Found one way to do it

darker --diff path/to/file.py | patch -p0 -s -o -

Edit: To also have if work it there are no changes one can do for example:

[[ -z $(darker --diff path/to/file.py) ]] && cat path/to/file.py || (darker --diff path/to/file.py | patch -p0 -s -o -)

If one puts this in a script file (darker-stdout):

#!/usr/bin/bash
DIFF=$(darker --diff $@)
if [[ -z $DIFF ]]; then
    cat $1
else
    echo "$DIFF" | patch -p0 -s -o -
fi

and in vim do

:%!darker-stdout %

to filter and update the buffer instead of writing to file. Or automatically on save with:

:autocmd BufWritePost *.py silent :%!darker-stdout % --isort

@levouh
Copy link
Collaborator

levouh commented Jul 16, 2021

@AckslD nice trick!

There's a typo in the regular command (not in the :h autocmd), that should be %!darker-stdout %, right?

Also, doing :silent! %!darker-stdout % prevents the confirmations to reload the buffer, etc.

@akaihola
Copy link
Owner

Hey @AckslD and @levouh, this sounds like a nice feature for some use cases. Do you think it still makes sense to add an option for this despite the patch trick? The option should obvioustly only work if a single Python source code file was submitted. What should the option be called?

@AckslD
Copy link
Author

AckslD commented Jul 16, 2021

I think it would still be a nice feature :) In eg isort it's called -d/--stdout

@AckslD
Copy link
Author

AckslD commented Jul 16, 2021

@levouh You mean without the --isort? I just wanted to show that arguments can still be passed :)

@akaihola
Copy link
Owner

In eg isort it's called -d/--stdout

Ok makes sense to re-use that.

The only gotcha would be if Black adopted either -d or --stdout for a different behavior. Using Darker as a drop-in replacement for Black would break in any IDE which starts to depend on that option. On the other hand, the probability is low since Black is already quite mature and all major IDEs already support it just fine as is.

@akaihola
Copy link
Owner

@AckslD, what should the behavior of -d / --stdout in your opinion be in the following cases:

  1. the user submits a single .py file, but it doesn't need any reformatting
  2. the user submits multiple .py files (or a directory with multiple files), and none of them needs reformatting
  3. the user submits multiple .py files (or a directory with multiple files), and one of them needs reformatting
  4. the user submits multiple .py files (or a directory with multiple files), and more than one of them needs reformatting
  5. the user submits a directory with only one .py file inside, and that file does need reformatting
  6. the user submits a directory with only one .py file inside, and that file does not need reformatting

akaihola added a commit that referenced this issue Jul 17, 2021
This still needs refining. Currently it prints out every file which was
changed by reformatting modified lines. We probably want to only print
out a single file.
@akaihola
Copy link
Owner

@AckslD, I started working on this in #165.

@AckslD & @levouh, would you like an invitation as a collaborator on this repository? I'd appreciate a code review on my changes.

As noted in the first commit,

This still needs refining. Currently it prints out every file which was changed by reformatting modified lines. We probably want to only print out a single file.

@levouh
Copy link
Collaborator

levouh commented Jul 17, 2021

Sure @akaihola, happy to review. I know clang-format supports this behavior so I'll take a look at how it handles multiple files when I have a second.

Grain of salt, but I think it's totally valid to only accept a single file. Not sure exactly what the use case would even be for handling multiple files through stdout anyways.


@levouh You mean without the --isort? I just wanted to show that arguments can still be passed :)

@AcksID I was noting that I've never seen the syntax :$!command % (as you'd put in your original post) and figured it was a typo for %!command %, but there's a pretty good chance I'm just not aware of the former. Either way, thanks for the tip!

@akaihola
Copy link
Owner

totally valid to only accept a single file

I think I agree here. So it would be a hard error if anything more than a single .py file path was provided together with -d/--stdout, even if it only led to one file being actually processed.

What about case 1., should -d/--stdout output the file even if reformatting caused no modifications?

I'd like to make these design choices at this point, since this requires a bit more logic than exists in the code right now.

@levouh
Copy link
Collaborator

levouh commented Jul 17, 2021

Yea, it'd be hard to distinguish which formatting goes with which file and whatnot without making your own format, etc. Easier for the user to just loop and know what's what it seems (unless you always output every file, but you'd still have to know where the files end).

What about case 1., should -d/--stdout output the file even if reformatting caused no modifications?

Not home right now, but this is what I was going to look at clang-format for (although it might make more sense to mirror what other Python tools like isort/black do). I'm pretty sure it outputs nothing, however all my wrapper scripts cat the file if the output from the formatter is empty so it seems logical to output the original file if it didn't change.

Rather than store_true, --stdout could take a value like empty/original. My opinion is biased towards my use case so I'm not sure this would be necessary, but it also doesn't seem hacky/dirty as its still just one argument.

@akaihola
Copy link
Owner

isort seems to output the original file if no modifications were done. This would support the decision for darker to do the same.

I believe black doesn't have an --stdout like mode at all.

Also, if darker would output nothing for unmodified files, the behavior would be ambiguous for empty files (e.g. a typical __init__.py) and ones which become empty when reformatted (ones with only space, tabs and newlines).

It's actually interesting that while black all_whitespace.py reports

All done! ✨ 🍰 ✨
1 file left unchanged.

running block.format_str() on the same non-empty all-whitespace content results in an empty zero-bytes output:

from black import FileMode, format_str

source = "\n\n   \n\t\n"
reformatted = format_str(source, mode=FileMode())
print(repr(reformatted))

So this seems to reveal a bug (?) where Darker and Black give different results when run on an edited all-whitespace file!

@akaihola
Copy link
Owner

akaihola commented Jul 19, 2021

Darker and Black give different results when run on an edited all-whitespace file

I found the reason for this. As hinted in my comment to psf/black#779, we could replace (in darker.black_diff.run_black()):

    return TextDocument.from_str(
        format_str(contents_for_black, mode=mode),
        encoding=src_contents.encoding,
        override_newline=src_contents.newline,
    )

with

    try:
        reformatted = format_file_contents(contents_for_black, fast=True, mode=mode)
    except NothingChanged:
        reformatted = contents_for_black
    return TextDocument.from_str(
        reformatted,
        encoding=src_contents.encoding,
        override_newline=src_contents.newline,
    )

to match black's behavior on all-whitespace files. The other option is to handle all-whitespace files in darker just like black does:

    contents_for_black = src_contents.string_with_newline("\n")
    if not contents_for_black.strip():
        return src_contents
    return TextDocument.from_str(
        format_str(contents_for_black, mode=mode),
        encoding=src_contents.encoding,
        override_newline=src_contents.newline,
    )

Which approach to choose should depend on Black's possible choice of a public Python API (psf/black#779).

@AckslD
Copy link
Author

AckslD commented Jul 19, 2021

@AcksID I was noting that I've never seen the syntax :$!command % (as you'd put in your original post) and figured it was a typo for %!command %, but there's a pretty good chance I'm just not aware of the former. Either way, thanks for the tip!

Oh yes, good spot, fix in the comment :)

@AckslD
Copy link
Author

AckslD commented Jul 19, 2021

What about case 1., should -d/--stdout output the file even if reformatting caused no modifications?

I think it should output the original file. I don't see any reason for not outputting anything in this case :) I think this flag should be "output what the file would be after running dark".

akaihola added a commit that referenced this issue Jul 21, 2021
This still needs refining. Currently it prints out every file which was
changed by reformatting modified lines. We probably want to only print
out a single file.
akaihola added a commit that referenced this issue Jul 30, 2021
Add the `-d` / `--stdout` option. Fixes #164.
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

3 participants