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

Option to select/unselect formats #4

Closed
pawamoy opened this issue Apr 14, 2024 · 14 comments
Closed

Option to select/unselect formats #4

pawamoy opened this issue Apr 14, 2024 · 14 comments

Comments

@pawamoy
Copy link

pawamoy commented Apr 14, 2024

I would like to unselect formatting of TOML snippets, because IMO the result is not that good:

[build-system]
requires = ["pdm-pep517"]
build-backend = "pdm.pep517.api"

[project]
name = "package-name"
description = "The package description."
version = "0.1.0"
authors = [{name = "Timothée Mazzucotelli", email = "[email protected]"}]
license = "ISC"
readme = "README.md"
requires-python = ">=3.7"
keywords = ["some", "keywords"]
classifiers = [
    "Development Status :: 4 - Beta",
    "Intended Audience :: Developers",
    "Programming Language :: Python",
    "Programming Language :: Python :: 3",
    "Programming Language :: Python :: 3 :: Only",
    "Typing :: Typed",
]

...is formatted to:

[build-system]
requires = [ "pdm-pep517",]
build-backend = "pdm.pep517.api"

[project]
name = "package-name"
description = "The package description."
version = "0.1.0"
license = "ISC"
readme = "README.md"
requires-python = ">=3.7"
keywords = [ "some", "keywords",]
classifiers = [ "Development Status :: 4 - Beta", "Intended Audience :: Developers", "Programming Language :: Python", "Programming Language :: Python :: 3", "Programming Language :: Python :: 3 :: Only", "Typing :: Typed",]
[[project.authors]]
name = "Timothée Mazzucotelli"
email = "[email protected]"

Things I don't like in the formatted block:

  • arrays start with a space, but do not end with one
  • arrays are inlined (line breaks removed) (this is the most crazy one 😅)
  • trailing commas are added to already inlined arrays
  • trailing commas are not removed after inlining arrays
  • inline mappings are moved after the current object
  • no space is added before a moved mapping

I suspect you don't do anything special in this project and just load and re-dump the TOML data.

So in the end I'd just like to prevent TOML formatting and just keep JSON and YAML 🙂

@KyleKing
Copy link

KyleKing commented Aug 22, 2024

That makes sense to me. Maybe all formatter dependencies could be moved to python extras, then if not present (e.g. ImportError), the package could print a warning and skip formatting that language?

While optional extras would work for YAML and TOML, they won't for JSON because it is built-in. Given that there haven't been any complaints, maybe the JSON formatter as non-optional and 2-spaces should be kept without changes. If considering configuration, maybe support for .editorconfig (https://pypi.org/project/EditorConfig) could be added (adding EditorConfig would be useful for configuring YAML formatting too!).

The current toml formatting library is actively developed (https://github.com/uiri/toml/commits/master), but hasn't had a new release since 2020. I personally prefer taplo (https://github.com/tamasfe/taplo, which has a Python wheel) for TOML formatting. Given that switching libraries would be a breaking change, maybe a good option is to accept PRs to extend mdformat-config to support third party formatters based on environment discovery (e.g. use taplo if present, then try toml if present, etc.). A concern with discovery is how to handle priority. For this, there could be a CLI flag, which would have the nice side-effect of enforcing that the third part formatter is present

Edited: clarified ideas for how EditorConfig could be used and general clarity

@hukkin
Copy link
Owner

hukkin commented Oct 15, 2024

Hi!

Yeah TOML formatting was subpar.
I wanted to keep things simple so I've simply switched from uiri/toml to taplo for TOML formatting. I hope it works for you, if not let's reopen the issue.

@hukkin hukkin closed this as completed Oct 15, 2024
@pawamoy
Copy link
Author

pawamoy commented Oct 15, 2024

Thanks, I'll try it again and report back 🙂

@pawamoy
Copy link
Author

pawamoy commented Oct 18, 2024

Unfortunately it fails to format my TOML code blocks.

% mdformat docs/work.md          
Warning: Failed formatting content of a toml code block (line 90 before formatting). Filename: /media/data/dev/copier-uv/docs/work.md
Warning: Failed formatting content of a toml code block (line 321 before formatting). Filename: /media/data/dev/copier-uv/docs/work.md
```toml title="config/ruff.toml"
[per-file-ignores]
"src/your_package/your_module.py" = [
    "T201",  # Print statement
]
```
```toml title="pyproject.toml"
[project]
dependencies = [
  "fastapi>=1.0",
  "importlib-metadata>=2.0",
]

[project.optional-dependencies]
test = [
  "pytest",
]

[tool.uv]
dev-dependencies = [
  "ruff",
]
```

Is that because of the title attribute on the first line?

@hukkin
Copy link
Owner

hukkin commented Oct 18, 2024

Hmm, I can't reproduce, works on my machine :)

I suspect you don't have taplo in your $PATH. Can you try to run

taplo

on the command line?

The taplo dependency should add it.

I could improve the error message to tell the user if taplo is not found.

@hukkin
Copy link
Owner

hukkin commented Oct 18, 2024

How did you install/run mdformat? What operating system did you use? If it's correct that you don't have taplo in your PATH, I'd like to try and understand why that happened...

@pawamoy
Copy link
Author

pawamoy commented Oct 18, 2024

Good intuition! I don't have taplo in my PATH. I installed mdformat with uv:

uv tool install mdformat --with mdformat-mkdocs --with mdformat-config

Unfortunately I believe uv does not yet support adding executable from dependencies into the PATH, like pipx does. Isn't there a way to call taplo from within the mdformat environment, so that it doesn't have to be on the PATH? For subprocesses we can usually do subprocess.run([sys.executable, "-m", "taplo", ...]).

@hukkin
Copy link
Owner

hukkin commented Oct 18, 2024

Ah yes, it seems

pipx install mdformat
pipx inject mdformat mdformat-config

doesn't work either.

I don't think subprocess.run([sys.executable, "-m", "taplo", ...]) works here, as taplo can't be run as a python module.

Not sure what's the best way to solve this. Probably some combination of

  • programatically finding the taplo binary
  • adding docs that tell the user that having taplo in PATH is required so they can install it themselves
  • better error message
  • using Docker as fallback in case the user has Docker installed

@hukkin
Copy link
Owner

hukkin commented Oct 18, 2024

But yeah for you I think e.g. uv tool install taplo separately should fix this.

@pawamoy
Copy link
Author

pawamoy commented Oct 18, 2024

Ha, right, the taplo executable is a binary file, the package doesn't install a single Python file in the env. Therefore I think option 1 is good: programatically finding the taplo binary. Shouldn't be too hard to find out the path of the current venv, and go look into its bin folder on Linux or Scripts folder on Windows (+ adding the .exe suffix). Maybe shutil.which already handles both cases. If it's all too bothersome, a docs and error message update is also great 😄

@pawamoy
Copy link
Author

pawamoy commented Oct 18, 2024

And thanks for your help @hukkin!

@hukkin
Copy link
Owner

hukkin commented Oct 18, 2024

Thanks for reporting!

@hukkin
Copy link
Owner

hukkin commented Oct 18, 2024

@pawamoy I've released version 0.2.1 where the pipx use case works for me. I don't have a Windows machine so couldn't test Windows, hope you're running linux or macos :)

@pawamoy
Copy link
Author

pawamoy commented Oct 18, 2024

Awesome, no more errors! Thanks a lot 🎉 (I'm on Arch btw)

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