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

semconvgen: Add a flag for a path to a custom capitalizations file #529

Conversation

carrbs
Copy link
Contributor

@carrbs carrbs commented Mar 16, 2024

Closes #528

This PR:

  • adds a new flag to semconvgen to read custom strings from a plaintext file
  • confirms the file is present in the filesystem
  • reads the custom strings from the given file (with error handling)
  • appends the strings to the static capitalizations slice

@carrbs carrbs requested review from a team and codeboten March 16, 2024 03:35
semconvgen/generator.go Outdated Show resolved Hide resolved
@carrbs carrbs changed the title [WIP] semconvgen: Add a flag for a path to a custom capitalizations file semconvgen: Add a flag for a path to a custom capitalizations file Apr 9, 2024
@carrbs
Copy link
Contributor Author

carrbs commented Apr 9, 2024

Hi @codeboten 👋 Do you have any thoughts on this? (I misread the CONTRIB file and thought I was supposed to add [WIP] to the title to solicit feedback when I opened the PR a few weeks ago 🤭).

Copy link

codecov bot commented Apr 10, 2024

Codecov Report

Attention: Patch coverage is 52.17391% with 11 lines in your changes are missing coverage. Please review.

Project coverage is 60.64%. Comparing base (dbdb565) to head (21a437c).

Files Patch % Lines
semconvgen/generator.go 52.17% 9 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #529      +/-   ##
==========================================
- Coverage   66.05%   60.64%   -5.41%     
==========================================
  Files          39       40       +1     
  Lines        1785     1964     +179     
==========================================
+ Hits         1179     1191      +12     
- Misses        461      626     +165     
- Partials      145      147       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

semconvgen/README.md Outdated Show resolved Hide resolved
Co-authored-by: Damien Mathieu <42@dmathieu.com>
@pellared pellared requested review from dmathieu April 10, 2024 13:58
semconvgen/generator.go Outdated Show resolved Hide resolved
semconvgen/generator.go Outdated Show resolved Hide resolved
semconvgen/generator.go Outdated Show resolved Hide resolved
semconvgen/generator.go Outdated Show resolved Hide resolved
Copy link
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

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

I think that we are not properly handling empty lines (or lines with only whitespaces).

Side note: I would not catch it without seeing the tests 😉

semconvgen/generator_test.go Outdated Show resolved Hide resolved
semconvgen/generator.go Show resolved Hide resolved
semconvgen/generator_test.go Show resolved Hide resolved
@pellared
Copy link
Member

ci/lint failed

@carrbs
Copy link
Contributor Author

carrbs commented Apr 15, 2024

ci/lint failed

@pellared I fixed this here: 2e3b1fe

@pellared
Copy link
Member

@carrbs Thanks for working on this👍

@pellared pellared merged commit a8785aa into open-telemetry:main Apr 19, 2024
10 of 12 checks passed
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.

semconvgen: Add a flag for a path to a custom capitalizations file
3 participants