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

docs: add formatter spec #32

Merged
merged 3 commits into from
Feb 2, 2021
Merged

docs: add formatter spec #32

merged 3 commits into from
Feb 2, 2021

Conversation

zimbatm
Copy link
Member

@zimbatm zimbatm commented Feb 1, 2021

Describe how formatters should ideally work.

Fixes #31

Describe how formatters should ideally work.
Copy link
Contributor

@basile-henry basile-henry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! 👍

Maybe it's a separate concern, but I do wonder: if we ever want prjfmt to have the option to check the format instead of applying the format, maybe this should make it into the specification.

If a formatter doesn't implement check functionality itself, that can be implemented in a wrapper script using diff (but then the formatter should probably not do in place formatting).

It might also make sense to not use the same options when in check mode. Some options might not make sense (for example ormolu's mode which can only be one of inplace, check or stdout). So maybe prjfmt could have a field check-args as well as args which would follow a similar specification but would only check if the files are formatted.


Where
* `<command>` is the name of the formatter.
* `[options]` is any number of flags and options that the formatter wants to
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prjfmt.toml calls these args, it might be better to keep these two names synchronized. How about we rename this to args or alternatively rename the field in prjfmt.toml.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea. I'm thinking we should do this the other way around and rename args to options. args is more generic and also includes the list of files that would be passed.

docs/formatter_spec.md Show resolved Hide resolved
@Rizary
Copy link
Contributor

Rizary commented Feb 2, 2021

I am still not sure whether we have to make a formatter spec or not. If the user still uses cargo fmt which does not adhere to the formatter spec, what's prjfmt's UX would be?

docs/formatter_spec.md Outdated Show resolved Hide resolved
Co-authored-by: Andika Demas Riyandi <andika.riyan@gmail.com>
@zimbatm
Copy link
Member Author

zimbatm commented Feb 2, 2021

Regarding the --check option, we already have that capability built into prjfmt thanks to Rizary's eval cache. If the mtime has changed after the formatting, it means that the files have changed. Files would be changed as well but it doesn't matter IMO.

@basile-henry
Copy link
Contributor

basile-henry commented Feb 2, 2021

Alright, so this relies on:

If, and only if, a file format has changed

Is that a common feature of formatters? I don't think ormolu does that for example, I think it'll write the file even if it's the exact same content.

@zimbatm
Copy link
Member Author

zimbatm commented Feb 2, 2021

We'll see. Thanks to the spec it should be possible to petition upstream formatters to implement the right thing. In the worse-case scenario, it's possible to fallback on the source control to query if any files are dirty after the formatting occurs. And then ship with some common implementations in the documentation. For example for git, it just adds a single line of code to the CI.

@zimbatm
Copy link
Member Author

zimbatm commented Feb 2, 2021

Another missing aspect of the spec is the XDG config part that formatters can ship.

Let's ship this and gather more experience by trying out a ton of formatters. We can always come back and update the spec.

@zimbatm zimbatm merged commit 9642ef2 into master Feb 2, 2021
@zimbatm zimbatm deleted the docs-formatter-spec branch February 2, 2021 10:25
brianmcgee pushed a commit that referenced this pull request May 13, 2024
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

Successfully merging this pull request may close these issues.

Write formatter spec
3 participants