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

improve fmt warning message #4444

Merged
merged 2 commits into from Dec 7, 2021
Merged

improve fmt warning message #4444

merged 2 commits into from Dec 7, 2021

Conversation

hrz6976
Copy link
Contributor

@hrz6976 hrz6976 commented Nov 25, 2021

Closes #4416
This PR only modifies "caddy fmt" warning message. The former one may cause confusion, and this mod might save several hours for someone else.

@CLAassistant
Copy link

CLAassistant commented Nov 25, 2021

CLA assistant check
All committers have signed the CLA.

@mholt
Copy link
Member

mholt commented Dec 2, 2021

The only qualm I have with this suggestion is that caddy fmt isn't just about a consistent style, sometimes it can fix bugs in the Caddyfile, for example missing a space where one is expected. It's not required, but it's more than just a style preference.

I'm really hoping nobody spends hours trying to figure out how to run caddy fmt though, if it takes more than a minute or two, there's something else wrong I think, and I'm not sure it's this warning string.

@francislavoie
Copy link
Member

IMO I think the warning string is confusing because it doesn't mention that caddy fmt is a command. Many users on the forums come to ask what it's about.

I think we could clarify, but I agree this PR's wording might not be the right one.

@mholt
Copy link
Member

mholt commented Dec 2, 2021

How about run the 'caddy fmt' command to fix formatting inconsistencies in the Caddyfile

@francislavoie
Copy link
Member

Or maybe: Caddyfile input is not formatted; run the 'caddy fmt' command to fix inconsistencies ?

@hrz6976
Copy link
Contributor Author

hrz6976 commented Dec 4, 2021

Thank you for your response to a newbie OSS developer! I think you got it right.

@hrz6976
Copy link
Contributor Author

hrz6976 commented Dec 4, 2021

Sorry for my newbie question :) To clean up the commit history, I rebased this branch with the upstream head, squashed the commits, and force-pushed. But I'm not quite sure how caddy does it (I've heard that force push is discouraged by some other projects, and if it is the case, what should I do?)

@francislavoie
Copy link
Member

We use GitHub's squash and merge button at the end usually. So it doesn't matter too much how PR authors end up organizing their branches in the end. What you did is fine 👍

@francislavoie francislavoie modified the milestones: 2.x, v2.5.0 Dec 7, 2021
@francislavoie francislavoie added the documentation 📚 Improvements or additions to documentation label Dec 7, 2021
Copy link
Member

@francislavoie francislavoie left a comment

Choose a reason for hiding this comment

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

Thanks!

Copy link
Member

@mholt mholt 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, thanks!

@mholt mholt merged commit e90d751 into caddyserver:master Dec 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation 📚 Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2.4.6: caddyfile with brackets can not be formatted
4 participants