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

Implement code generation for config parsing. #1893

Merged
merged 1 commit into from
May 31, 2024
Merged

Conversation

kislaykishore
Copy link
Collaborator

  • Create a new package called "cfg" where the generated code will reside. In future, this code will be moved into the config package to complete the migration.
  • Add an import in main.go to force compile the new package. Otherwise, we'll not know of any compile errors in the codegen and generated code.
  • This will also enable developers to add new flags to params.yaml thereby reducing divergence with the current setup.

Description

Link to the issue in case of a bug fix.

NA

Testing details

  1. Manual - NA
  2. Unit tests - we'll add tests for config parsing in a subsequent PR.
  3. Integration tests - NA

@kislaykishore kislaykishore requested a review from a team as a code owner May 8, 2024 04:45
@kislaykishore kislaykishore requested review from sethiay and ashmeenkaur and removed request for sethiay May 8, 2024 04:45
@kislaykishore kislaykishore force-pushed the cli-cfg-parity2 branch 12 times, most recently from 09682f9 to 672a22b Compare May 8, 2024 07:09
main.go Outdated Show resolved Hide resolved
codegen/main.go Outdated Show resolved Hide resolved
@kislaykishore kislaykishore force-pushed the cli-cfg-parity2 branch 4 times, most recently from ee97d8f to 07ace31 Compare May 8, 2024 14:50
codegen/main.go Outdated Show resolved Hide resolved
codegen/main.go Outdated Show resolved Hide resolved
codegen/main.go Outdated Show resolved Hide resolved
codegen/main.go Outdated Show resolved Hide resolved
codegen/main.go Outdated Show resolved Hide resolved
codegen/main.go Outdated Show resolved Hide resolved
codegen/main.go Outdated Show resolved Hide resolved
codegen/main.go Outdated Show resolved Hide resolved
codegen/main.go Outdated Show resolved Hide resolved
codegen/main.go Outdated Show resolved Hide resolved
codegen/main.go Outdated Show resolved Hide resolved
codegen/main.go Outdated Show resolved Hide resolved
codegen/main.go Outdated Show resolved Hide resolved
codegen/main.go Outdated Show resolved Hide resolved
codegen/main.go Outdated Show resolved Hide resolved
codegen/main.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@ashmeenkaur ashmeenkaur left a comment

Choose a reason for hiding this comment

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

I've completed a first pass review of the code. I've added some comments, mainly focused on improving the readability of the code. I apologize if there are quite a few – I hope they're helpful!

I also noticed a potential opportunity to refactor codegen/main.go. It might be beneficial to create separate utility files for:

  • Parsing the input params.yaml file
  • Generating type template data

This could help streamline the main file and make the logic easier to follow.

Overall, I found some parts of the code a little challenging to understand at first. Please see if you can do something to make it more intuitive :)

ashmeenkaur
ashmeenkaur previously approved these changes May 28, 2024
cfg/config.go Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
ashmeenkaur
ashmeenkaur previously approved these changes May 28, 2024
Copy link

codecov bot commented May 28, 2024

Codecov Report

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

Project coverage is 73.55%. Comparing base (5114551) to head (25cb4d1).

Files Patch % Lines
cfg/config.go 0.00% 31 Missing ⚠️
cfg/types.go 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1893      +/-   ##
==========================================
- Coverage   73.76%   73.55%   -0.21%     
==========================================
  Files          93       95       +2     
  Lines       10219    10252      +33     
==========================================
+ Hits         7538     7541       +3     
- Misses       2347     2378      +31     
+ Partials      334      333       -1     
Flag Coverage Δ
unittests 73.55% <0.00%> (-0.21%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

* Create a new package called "cfg" where the generated code will reside. In future, this code will be moved into the config package to complete the migration.
* Add an import in main.go to force compile the new package. Otherwise, we'll not know of any compile errors in the codegen and generated code.
* Add a limited set of params in params.yaml to verify that the code generation works. We'll add the entire set of params in a subsequent PR.
@kislaykishore kislaykishore merged commit f36979a into master May 31, 2024
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.

None yet

2 participants