Force single or double quotes #304
Conversation
This PR adds `force_quote_style` to the basic formatter. This will allow users to for `'` or `"` around all strings. Along with this I ended up doing some refactoring for config validation to make it so invalid values for some enum options will actually fail instead of just silently doing nothing. I also hit the last straw with the tests in this package so I totally redid them. I could have done it in another PR so this one isn't too big, but I'm the only person who works on this so as the only person inconvenienced I say it's fine. :)
braydonk
force-pushed
the
force_quote_type
branch
from
21cb420 to
94167c8
Compare
| "github.com/google/yamlfmt/formatters/basic" | ||
| "github.com/google/yamlfmt/formatters/basic/features" | ||
| "github.com/google/yamlfmt/internal/assert" | ||
| "github.com/stretchr/testify/require" |
There was a problem hiding this comment.
Hi @braydonk
I'm a random Gopher watching your repository, but I'm also one of the testify maintainers. I mention it to say I'm not against the usage of testify usually.
I'm a bit surprised you used testify and so added a bunch of dependencies to your go.mod while you have a
github.com/google/yamlfmt/internal/assert package you could have updated slightly to provide the require.ErrorIs and require.NoError you used.
There was a problem hiding this comment.
Hi @ccoVeille,
I decided while I was writing this PR that I'm kinda tired of the assertion package I wrote and am not super interested in continuing to work on it. You're right that adding the other types of assertions in my assert package would have been fine, but the main reasons I'm not all that excited about maintaining it anymore is because:
- It takes work to produce nice error messages out of each new assertion pattern
- Eventually I'm hoping to increase external contribution to this repository, and it's a little embarrassing to tell people to use the rinkydink assertion package I wrote for fun in a weekend and stuck with in a major project
In all the other repos I work on they happen to use testify's require package, so I got super used to writing the assertions and reading the failure messages. The extra deps in the Go mod are a bit of a pain, but since they won't get built into the resulting binary, I figured it was a fine cost. Over the holidays I may scrub all usages of the internal assert package and use require instead.
Is there a particular other reason it's worth avoiding testify other than the go module clutter?
There was a problem hiding this comment.
I'm fine with the usage of testify of course.
My comment was mainly triggered by the fact I saw testify being added to the go.mod while reviewing the PR I had interest in.
I just wanted to make sure it was something you expected to do.
Sometimes IDEs import things by there own because we are used to write require.NoError and then testify is imported.
I wanted to check if it was intentional or not.
Here it's clearly a motivated move. So it's good.
There was a problem hiding this comment.
The extra deps in the Go mod are a bit of a pain, but since they won't get built into the resulting binary, I figured it was a fine cost.
For records we worked on reducing the number of external libraries in the future version (not planned for now)
Over the holidays I may scrub all usages of the internal assert package and use require instead.
👍
There was a problem hiding this comment.
I just wanted to make sure it was something you expected to do. Sometimes IDEs import things by there own because we are used to write require.NoError and then testify is imported.
Ah I see, I think that has happened to me with the assert name making my IDE try to pull in testify and gotest.tools lol
Yeah this was a fully intentional switch. I had thought about doing it sweeping across the whole repo for a while, it's just the kind of general refactoring change I don't get much time to make on yamlfmt these days. Since I needed to refactor tests for this package in particular I decided to make the switch here on its own.
2 participants
Closes #288
This PR adds
force_quote_styleto the basic formatter. This will allow users to for'or"around all strings. Along with this I ended up doing some refactoring for config validation to make it so invalid values for some enum options will actually fail instead of just silently doing nothing. I also hit the last straw with the tests in this package so I totally redid them. I could have done it in another PR so this one isn't too big, but I'm the only person who works on this so as the only person inconvenienced I say it's fine. :)