- 
                Notifications
    You must be signed in to change notification settings 
- Fork 2.1k
cli/command/container: inline some variables #6536
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
Conversation
| Codecov Report❌ Patch coverage is  
 📢 Thoughts on this report? Let us know! | 
Signed-off-by: Sebastiaan van Stijn <[email protected]>
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.
Pull Request Overview
This PR refactors variable declarations in the container options parsing code to inline variable assignments where appropriate, improving code readability and reducing unnecessary variable declarations.
- Inlined variable declarations for port parsing operations
- Improved slice initialization with capacity hints for device mappings
- Reduced variable scope by moving declarations closer to usage
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| deviceMappings := make([]container.DeviceMapping, 0, len(devices)) | ||
| cdiDeviceNames := make([]string, 0, len(devices)) | 
    
      
    
      Copilot
AI
    
    
    
      Oct 6, 2025 
    
  
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.
Consider using nil initialization for these slices instead of pre-allocating with capacity. Since you're only appending selectively (CDI devices to cdiDeviceNames, validated devices to deviceMappings), the actual usage may be much less than the full capacity, making pre-allocation potentially wasteful.
| deviceMappings := make([]container.DeviceMapping, 0, len(devices)) | |
| cdiDeviceNames := make([]string, 0, len(devices)) | |
| var deviceMappings []container.DeviceMapping | |
| var cdiDeviceNames []string | 
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.
LOL; go fight that battle with the linter, CoPilot, I changed it because the linter didn't like your suggestion 😜
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 always have to read through these to understand if the feedback is anything worth actually actioning or not.
- Human readable description for the release notes
- A picture of a cute animal (not mandatory but encouraged)