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

Support reading configuration options from a file #263

Closed
3 tasks
rpdelaney opened this issue Sep 7, 2021 · 8 comments · Fixed by #264
Closed
3 tasks

Support reading configuration options from a file #263

rpdelaney opened this issue Sep 7, 2021 · 8 comments · Fixed by #264
Labels
enhancement New feature or request

Comments

@rpdelaney
Copy link
Contributor

Description / Summary

mdformat supports three configuration options via switches to the command line interface:

  --number              apply consecutive numbering to ordered lists
  --wrap {keep,no,INTEGER}
                        paragraph word wrap mode (default: keep)
  --end-of-line {lf,crlf}
                        output file line ending mode (default: lf)

However, there does not seem to be support for configuring these options from a file that could be committed to source control.

Value / benefit

A configuration file would assist in ensuring consistent behavior between executing manually in a terminal, an editor integration, and when running as a pre-commit hook.

Implementation details

I find yaml files to be excellent for implementing configurations that are more often parsed by humans than by programs. But I do not have a strong opinion about this. toml is also an option that is gaining traction in the python ecosystem.

To discover the configuration file, I suggest this logic to follow the xdg base directory spec:

  1. Check ./.mdformat-config.yaml
  2. If not found, check $XDG_CONFIG_HOME/mdformat-config.yaml
  3. If not found, check $HOME/mdformat-config.yaml
  4. If not found, check $HOME/.mdformat-config.yaml

If command-line switches are passed, they should over-ride conflicting options in the configuration file.

Tasks to complete

  • Decide on a configuration file format.
  • Decide on a discovery path for the configuration file.
  • Code it!
@rpdelaney rpdelaney added the enhancement New feature or request label Sep 7, 2021
@welcome
Copy link

welcome bot commented Sep 7, 2021

Thanks for opening your first issue here! Engagement like this is essential for open source projects! 🤗

If you haven't done so already, check out EBP's Code of Conduct. Also, please try to follow the issue template as it helps other community members to contribute more effectively.

If your issue is a feature request, others may react to it, to raise its prominence (see Feature Voting).

Welcome to the EBP community! 🎉

@hukkin
Copy link
Member

hukkin commented Sep 7, 2021

Hey, thanks for the issue!

I recommend using the open source pre-commit tool and its configuration file (.pre-commit-config.yaml) to configure mdformat. It's a great match for mdformat. Not only does it let you configure mdformat CLI args, but also things like file inclusion and exclusion patterns, required plugins and their versions, required mdformat version etc. Would this work for you?

I've been trying to avoid, or at least postpone having an mdformat specific conf file as
a) mdformat isn't very configurable, and
b) it introduces complexity (file discovery logic, CLI overriding, parser library, etc.) that if we can delegate to a purpose-built tool like pre-commit, why not do so?

If there's use cases that pre-commit won't solve and other users also need this, then sure let's have a conf file. In that case the format will be TOML and filename will be .mdformat.toml. Regarding file discovery, I'd want to research what other formatters like Prettier, Black or Rustfmt do before making any decisions.

@rpdelaney
Copy link
Contributor Author

Thanks for the thoughtful response!

Would this work for you?

I do use pre-commit as much as possible, and I try to introduce it where it is not already in use. It is an excellent tool. However, this is exactly where my problem lies. In the issue I wrote, somewhat nebulously:

A configuration file would assist in ensuring consistent behavior between executing manually in a terminal, an editor integration, and when running as a pre-commit hook.

I'll describe my particular use case in more detail in an effort to illustrate what I mean by this.

I write code in vim, and make use of many plugins and custom bindings to assist with this. For example, when editing python files, pressing F5 blackens the code in the focused buffer. When editing JSON, F5 now formats it with python's json.tool. When editing golang, F5 runs gofmt. Etc. Naturally, when I discovered mdformat, I added a similar bind for markdown files.

The problem here is that when executed in a shell, mdformat does not read local configuration from the pre-commit config. So if any options are set there, I will often be running into failed hooks when I attempt to commit my changes even if I already ran mdformat from the editor. If I'm editing a lot of markdown that day, that's a lot of extra toil and tedium.

black's vim plugin actually suffers from a similar problem, which I described in this open issue: psf/black#1587

I hope this helps.

a) mdformat isn't very configurable

Unfortunately, having any configuration options at all exposes us to this problem.

b) it introduces complexity (file discovery logic, CLI overriding, parser library, etc.) that if we can delegate to a purpose-built tool like pre-commit, why not do so?

I agree and completely sympathize with your concerns about complexity. Even if I were to help write it initially, I am not volunteering to maintain all that new code indefinitely. :-) My objection is to everything after "that if...". pre-commit is not a configuration file because your other documented execution paths (through the terminal, and the python API) do not read options from it.

In that case the format will be TOML and filename will be .mdformat.toml. Regarding file discovery, I'd want to research what other formatters like Prettier, Black or Rustfmt do before making any decisions.

Simply reading ./.mdformat.toml and calling it a day would be plenty enough to close this issue from my POV.

@hukkin
Copy link
Member

hukkin commented Sep 8, 2021

Regarding running mdformat in vim and terminal: what if instead of running

mdformat <filename>

you run (and configure vim to run)

pre-commit run mdformat --files <filename>

This should make it so that pre-commit's configuration is used in those environments also. What do you think?

Unfortunately, having any configuration options at all exposes us to this problem.

*sigh* yeah you're right about that.

@rpdelaney
Copy link
Contributor Author

This should make it so that pre-commit's configuration is used in those environments also. What do you think?

That might enable my current workflow. But there is a performance penalty:

$ time pre-commit run mdformat --files rules.md
mdformat.................................................................Passed

real    0m2.424s
user    0m0.801s
sys     0m0.276s
$ time mdformat rules.md

real    0m0.480s
user    0m0.273s
sys     0m0.061s

I still feel constrained in how I can use this tool and promote its use in other projects that I don't control, though.

Anyway, I understand if you decide not to do this. Thanks for hearing me out.

@hukkin
Copy link
Member

hukkin commented Sep 9, 2021

Hmm, wonder why the performance hit was so significant for you. Perhaps the virtual environment wasn't initialized or not everything was installed on that 2.424 second invocation? Is the difference as significant on following invocations as well? On my machine I get

foo@bar:~/dev/tomli$ time pre-commit run mdformat --files README.md CHANGELOG.md 
mdformat.................................................................Passed

real    0m0.514s
user    0m0.468s
sys     0m0.050s
foo@bar:~/dev/tomli$ time mdformat README.md CHANGELOG.md 

real    0m0.487s
user    0m0.462s
sys     0m0.024s

There's barely any difference. I ran against tomli repository, with same plugins (the ones listed in pre-commit-config.yaml) in both methods.


I studied conf file discovery logic of a couple other tools. I think what I'd want to do here is simply walk from current working directory to file system root, stopping if the file is found. This is more or less what rustfmt does. Prettier walks up from the directory of the file being formatted instead of CWD, which I also really like, but don't want to deal with the cases when more than one files, no files, or symlinks are passed in. Black seems to determine a common ancestor directory of the files passed in and start from that, which I find slightly more complex but not necessarily that much better than starting from CWD.

Still not 100% sure we need the conf file, but if yes, it should be more or less planned out now 😄

@rpdelaney
Copy link
Contributor Author

I think what I'd want to do here is simply walk from current working directory to file system root, stopping if the file is found.

That would be perfect! The discovery path I suggested in the OP would be for if we want to support user-specific configurations as well, but in retrospect that does seem to be somewhat against the spirit of this tool.

@hukkin
Copy link
Member

hukkin commented Sep 10, 2021

Ok I've warmed up to the idea, mainly because

a) mdformat isn't very configurable

Unfortunately, having any configuration options at all exposes us to this problem.

And also, perhaps not every user is able to use git and pre-commit.

So a PR is very welcome if you or anyone else is interested in working on this. If not, I may do this eventually myself.

I've changed my mind slightly regarding file discovery: let's do what Prettier does, i.e.:

  • Walk to root starting from the directory of the file to be formatted. If there are more than one files to be formatted, discover a conf file for them individually. Smart use of functools.lru_cache should make this a non-issue performance wise.
  • If the path is - (meaning stdin) only then start from CWD.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants