Skip to content

Conversation

TekuSP
Copy link
Contributor

@TekuSP TekuSP commented Sep 21, 2025

Nuspecs are there, but most are not published.

It is missing for my usages, I have fixed the script hopefully. Please someone review, and merge, and make stable patch release.
Thank you.

Refactored the Invoke-BuildNanoNugets function to use a loop for packing nuget files instead of individual commands.
Copy link

claude bot commented Sep 21, 2025

Code Review for PR #1625

Summary

This PR fixes the nanoFramework NuGet package build script to automatically discover and pack all available nuspec files instead of hardcoding a specific list. This is a good improvement that makes the build process more maintainable.

Positive Changes

  • Dynamic discovery: The script now automatically finds all .nuspec files using Get-ChildItem, eliminating the need to manually maintain a list
  • Future-proof: New quantities added to nanoFramework will automatically be packaged without script modifications
  • Cleaner code: Reduces 23 lines to 5 lines, making the script more maintainable

Issues to Address

  1. PowerShell variable naming conflict on line 9. The loop variable $nuget shadows the $nuget executable path variable. Fix:
    Change: Foreach ($nuget in $nugetsToProcess)
    To: Foreach ($nuspecFile in $nugetsToProcess)
    And update the pack command accordingly.

  2. Unnecessary echo command on line 5. The echo is redundant in PowerShell.
    Change: | % { echo $_.FullName }
    To: Use .FullName property directly

Impact

  • Breaking changes: None
  • Performance: Neutral
  • Test coverage: Build scripts tested through CI/CD
  • Security: No concerns

Recommendation

Approve after fixes - The approach improves maintainability, but the PowerShell syntax errors must be fixed before merging.

Copy link

claude bot commented Sep 21, 2025

Code Review

Summary

This PR fixes a critical issue where only 21 out of 129 nanoFramework NuGet packages were being published. The change replaces hardcoded package paths with a dynamic discovery mechanism that automatically finds and packs all .nuspec files.

Key Improvements ✅

  • Scalability: The new approach automatically discovers all 129 .nuspec files instead of hardcoding 21 specific packages
  • Maintainability: No manual updates needed when new quantities are added to the library
  • Completeness: All nanoFramework packages will now be published, not just a subset

Code Quality

The PowerShell implementation is clean and follows good practices:

  • Uses Get-ChildItem with recursive search to find all nuspec files
  • Maintains the same verbosity settings for debugging
  • Adds helpful echo statement for build output visibility

No Breaking Changes

This is a build script change that only affects the packaging process. No API or functionality changes.

Minor Suggestions

  1. Consider adding error handling for the nuget pack command:

    if ($LASTEXITCODE -ne 0) {
        Write-Error "Failed to pack: $nuspecFile"
    }
  2. The echo statement has a typo: "Succesfully" → "Successfully"

Recommendation

Approved ✅ - This is an important fix that ensures all nanoFramework packages are properly published. The dynamic approach is much more maintainable than the previous hardcoded list.

@TekuSP
Copy link
Contributor Author

TekuSP commented Sep 21, 2025

I have fixed the non-existing generated files. We should probably clear directories before running generator again.

@TekuSP
Copy link
Contributor Author

TekuSP commented Sep 21, 2025

@angularsen Can I please ask for review and merge? I really miss a lot of nanoframework packages.

Copy link

claude bot commented Sep 21, 2025

Code Review

Thank you for this contribution! This is a good improvement that dynamically discovers all nuspec files instead of hardcoding them.

✅ Strengths

  • Smart automation: Replacing 21 hardcoded nuget pack commands with a dynamic discovery approach is much more maintainable
  • Future-proof: New nanoFramework packages will be automatically included without script updates
  • Clean implementation: The PowerShell approach using Get-ChildItem with recursive filtering is idiomatic

🔍 Observations

  1. Removed files: The PR removes LapseRate and TorquePerLength nanoFramework projects/nuspecs. These appear to be unused or obsolete quantities that were removed from generation.

  2. UnitRelations.json changes: Two lines were reordered but remain functionally identical - likely from code generation sorting.

  3. Coverage: The script will now package all 129 nanoFramework nuspec files automatically (vs the previous 21 hardcoded ones).

✅ No Breaking Changes

This is an internal build script change with no impact on the public API or published packages.

🎯 Recommendation

Approved - This is a straightforward improvement to the build process that will prevent missing packages in future releases.

The maintainers should merge this and create a patch release as requested to ensure all nanoFramework packages are published.

@angularsen angularsen merged commit 3415e3c into angularsen:master Sep 26, 2025
1 check 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.

3 participants