Simplify external configuration settings #162
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #162 +/- ##
==========================================
+ Coverage 67.93% 67.97% +0.04%
==========================================
Files 73 73
Lines 3739 3747 +8
==========================================
+ Hits 2540 2547 +7
- Misses 868 869 +1
Partials 331 331
Continue to review full report at Codecov.
|
CHANGELOG.md
Outdated
| @@ -8,6 +8,11 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. | |||
| - Delete the ability to explicitly specify multiple files, and have the effect | |||
| of one file being specified be the same as the former `--dir-mode`. See | |||
| [#16](https://github.com/uber/prototool/issues/16) for more details. | |||
| - Remove protoc_include_wkt setting. This is always set to true. | |||
There was a problem hiding this comment.
Stupid things:
- s/Remove/Delete/ to be consistent with above
- Put tickmarks around each option name ie
protoc_include_wkt.
There was a problem hiding this comment.
Sure, sounds good!
| @@ -1,3 +1,14 @@ | |||
| protoc_include_wkt: true | |||
| lint: | |||
| group: all | |||
There was a problem hiding this comment.
Ah shoot, this is why I had it be a soft failure if lint.group is specified - this test tests if all linters are turned on. I'm not sure what to do here - we could have all these options be an error if not in devel mode? But then you have to propagate development mode all the way down, which I don't like...I don't know ugh.
I'm moderately confused as to how tests are passing if you remove this.
There was a problem hiding this comment.
I fixed the test by adding all of the missing linters under include_ids. This way, we mimic the all setting because it's essentially: Default + (All - Default) == All
There was a problem hiding this comment.
Ah got it...well not optimal but hey, works for now.
internal/settings/settings.go
Outdated
| // - Lint.Group | ||
| // - Gen.GoOptions.NoDefaultModifiers | ||
| func (e ExternalConfig) Validate() error { | ||
| if e.NoDefaultExcludes { |
There was a problem hiding this comment.
Actually, we're going to want to make this the new default, and make this true, or alternatively remove anything from the list of default excludes in https://github.com/uber/prototool/blob/dev/internal/settings/settings.go#L49
There was a problem hiding this comment.
Ah yea good point. Nice catch, I'll update.
|
@peter-edge All comments addressed. |
CHANGELOG.md
Outdated
| - Delete `no_default_excludes` setting. This is always set to true. | ||
| - Delete `gen.go_options.no_default_modifiers` setting. | ||
| - Delete `lint.group` setting. | ||
| - Delete `create.dir_to_base_package` -> `create.dir_to_package`. |
There was a problem hiding this comment.
Nit: this is really more renamed, I can clean it up before release though.
There was a problem hiding this comment.
Ah yea, that was a bad update on my end. I'll update before rebasing / merging into dev.
| @@ -1,3 +1,14 @@ | |||
| protoc_include_wkt: true | |||
| lint: | |||
| group: all | |||
There was a problem hiding this comment.
Ah got it...well not optimal but hey, works for now.
|
Wanted to provide some feedback that this breaks our working v0.4.0 configuration. I understand the project is in pre-1.0, but it would be helpful if breaking changes in the external API along with migration notes, for example, in the readme or changelog. 😄 |
|
All changes have been noted in the changelog. We plan to release v1.0 soon, but before v1.0, use is at your own risk and we take no responsibility for breaking changes. |
This un-registers and renames the external configuration settings according to the following:
Note that this PR is currently branched off of #161. Many of the testdata file directories are updated there, so this PR can be more-easily merged in afterwards.