-
Notifications
You must be signed in to change notification settings - Fork 16
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
Identify all tag styles #1367
Identify all tag styles #1367
Conversation
Breaking into smaller, more specific functions.
At least one of the assigning values should match the {key:string, value:string} style.
Does the PR have any schema changes?Looking good! No breaking changes found. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1367 +/- ##
==========================================
+ Coverage 22.13% 22.38% +0.25%
==========================================
Files 24 24
Lines 4103 4124 +21
==========================================
+ Hits 908 923 +15
- Misses 3044 3049 +5
- Partials 151 152 +1 ☔ View full report in Codecov by Sentry. |
5a1a05a
to
4771a61
Compare
var unexpectedTagsShapes map[string]interface{} | ||
err = json.Unmarshal(bytes, &unexpectedTagsShapes) | ||
require.NoError(t, err) | ||
assert.Empty(t, unexpectedTagsShapes, "reports/unexpectedTagsShapes.json should be empty, which means that we need to update GetTagsStyle in gen_tags.go to cover new tags variations.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want to fail hard at build time, why not just directly in GetTagsStyle? So we have the complete report to look at?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah - would prefer not failing during the build, but after in the test – having all the issues laid out in the report and the ability to fix one at a time if needed. Also quite easy to disable the test temporarily but still have the information reported.
The purpose of complete coverage of all tags styles is to maintain quality of tag accessibility. Additionally, this will allow us to reliably apply DefaultTags in a future PR (see #107).