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

Doesn't work with certain files #52

Closed
Bobronium opened this issue May 18, 2022 · 9 comments
Closed

Doesn't work with certain files #52

Bobronium opened this issue May 18, 2022 · 9 comments
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug info-needed Issue requires more information from poster needs investigation

Comments

@Bobronium
Copy link

black -S --stdin-filename /Users/bobronium/Library/Application Support/Code/User/globalStorage/buenon.scratchpads/scratchpads/1a4f2b73c916c9af3926773a674c6453/scratch.py -
CWD Formatter: /Users/bobronium/dev/py/...
[Error - 1:45:24 AM] No Python files are present to be formatted. Nothing to do 😴

I'm not sure if it's an actual cmd, or its representation, but I can see right away that the file path is not escaped/quoted, which might cause this issue.

@github-actions github-actions bot added the triage-needed Issue is not triaged. label May 18, 2022
@karthiknadig
Copy link
Member

@Bobronium That is just a representation, the path itself is passed in programmatically via sys.argv when processing the request. See here:

LSP_SERVER.show_message_log(" ".join(argv))

Can you share the settings you are using?

@karthiknadig
Copy link
Member

We use stdin to actually send in the contents of the file. If you want to manually try this out the equivalent command from the terminal would be:

cat <filepath> | python -m black -S --stdin-filename <filepath> - 

you may have to quote the paths when actually running from the terminal.

@karthiknadig karthiknadig added bug Issue identified by VS Code Team member as probable bug info-needed Issue requires more information from poster needs investigation and removed triage-needed Issue is not triaged. labels May 18, 2022
@karthiknadig karthiknadig self-assigned this May 18, 2022
@Bobronium
Copy link
Author

Bobronium commented May 20, 2022

Can you share the settings you are using?

    "[python]": {
        "editor.defaultFormatter": "ms-python.black-formatter",
    },
    "black-formatter.args": [
        "-S"
    ],

Upd.

cat "/Users/bobronium/Library/Application Support/Code/User/globalStorage/buenon.scratchpads/scratchpads/1a4f2b73c916c9af3926773a674c6453/scratch..py" | python -m black -S --stdin-filename "/Users/bobronium/Library/Application Support/Code/User/globalStorage/buenon.scratchpads/scratchpads/1a4f2b73c916c9af3926773a674c6453/scratch..py" -
No Python files are present to be formatted. Nothing to do 😴

But

python -m black -S "/Users/bobronium/Library/Application Support/Code/User/globalStorage/buenon.scratchpads/scratchpads/1a4f2b73c916c9af3926773a674c6453/scratch..py"
All done! ✨ 🍰 ✨

So, I guess it's an issue with black then?

@Bobronium Bobronium changed the title Doesn't work when path contains whitespace Doesn't work with certain files May 20, 2022
@ichard26
Copy link

Maintainer of Black here 👋

Yeah I think Black is mishandling --stdin-filename here. Could you try adding --verbose to black-formatting.args and rerun this extension and see what additional logs it spits out @Bobronium ?

@Bobronium
Copy link
Author

Bobronium commented May 20, 2022

Here's the line where path is lost https://github.com/psf/black/blob/9ce100ba61b6738298d86818a3d0eee7b18bfed7/src/black/__init__.py#L626

Or, to be more precise:
https://github.com/psf/black/blob/9ce100ba61b6738298d86818a3d0eee7b18bfed7/src/black/files.py#L164

ValueError says '/Users/bobronium/Library/Application Support/Code/User/globalStorage/buenon.scratchpads/scratchpads/1a4f2b73c916c9af3926773a674c6453/scratch.py' is not in the subpath of '/Users/bobronium/dev/py/pythings' OR one path is relative and the other is absolute.

@ichard26
Copy link

ichard26 commented May 20, 2022

Sounds like Black is mishandling the project root in STDIN mode. I can look into this later today. A list of reproduction steps would be nice, but it seems like an easy enough issue to reproduce?

@Bobronium
Copy link
Author

Here you go!

  1. Create file outside of the project
echo "print('Hello, world!')" > outside.py
  1. Create a project with black installed
poetry new project && cd project && poetry add black && poetry shell
  1. Try to format file with --stdin-filename
cat ../outside.py | black --stdin-filename "../outside.py" -
No Python files are present to be formatted. Nothing to do 😴
  1. Format it regularly
black "../outside.py"
reformatted ../outside.py

All done! ✨ 🍰 ✨
1 file reformatted.

@Bobronium
Copy link
Author

For another issue, this time with the extension iteslf

I wasn't getting any output to the console or any indication that anything is happening while trying to format certain files.

Had to hack in vscode-black-formatter/bundled/formatter/format_server.py to see what's going on
(both LSP_SERVER.show_message_log were added by me):

def _format(
    params: types.DocumentFormattingParams,
) -> Union[List[types.TextEdit], None]:
    """Runs formatter, processes the output, and returns text edits."""
    document = LSP_SERVER.workspace.get_document(params.text_document.uri)
    LSP_SERVER.show_message_log(f"Formatting... {document}")

    
    if utils.is_stdlib_file(document.path) or not is_python(document.source):
        LSP_SERVER.show_message_log(f"{utils.is_stdlib_file(document.path)=}, {not is_python(document.source)=}")
        # Don't format standard library python files. Or, invalid python code
        # or non-python code in case of notebooks
        return None

Got this output:

Formatting... file:///Users/bobronium/dev/py/pythings/tests/conftest.py
utils.is_stdlib_file(document.path)=False, not is_python(document.source)=True

is_python returns False when ast.parse(document.source) fails. But why this check would exist in the first place? Black already does it, and outputs:

black "/Users/bobronium/dev/py/pythings/tests/conftest.py"
error: cannot format /Users/bobronium/dev/py/pythings/tests/conftest.py: Cannot parse: 23:4:     item.add_marker("asyncio")

Oh no! 💥 💔 💥
1 file failed to reformat.

Not only it indicates the error, but also shows the exact line that couldn't be parsed.

My proposal:

  1. Never exit without any output in any case. Always tell what's happening, e.g. "Detected standard library, exiting without reformatting...".
  2. Drop is_python check and let black do its thing
  3. Write something to the status bar when extension is called to indicate that wheels are turning

@karthiknadig
Copy link
Member

Closing in favor of #55

@karthiknadig karthiknadig closed this as not planned Won't fix, can't repro, duplicate, stale Dec 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue identified by VS Code Team member as probable bug info-needed Issue requires more information from poster needs investigation
Projects
None yet
Development

No branches or pull requests

3 participants