Recognizes version variables and updates the version accordingly#478
Recognizes version variables and updates the version accordingly#478IT-VBFK wants to merge 1 commit into
Conversation
2e73450 to
1391641
Compare
ITaluone
added
enhancement
f157a46 to
40c5ed9
Compare
There was a problem hiding this comment.
Pull request overview
Updates Fallout.Migrate’s .csproj rewrite logic to handle projects that pin Fallout/Nuke package versions via MSBuild properties (e.g., Version="$(NukeVersion)"), aligning with issue #477’s migration expectations.
Changes:
- Refines the “inline literal version” rewrite to avoid consuming
Version="$(...)"so variable-based versions can be handled separately. - Adds a new pass that discovers version variables used by PackageReferences and bumps the corresponding property values to the target Fallout version.
- Extends specs to cover
NukeVersion→FalloutVersionrenaming and arbitrary version-variable bumping behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/Fallout.Migrate/CsprojRewriter.cs |
Adds variable-aware version handling and bumps property-based versions to the target Fallout version. |
tests/Fallout.Migrate.Specs/CsprojRewriterSpecs.cs |
Adds/adjusts specs to validate migration behavior for version variables. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
da9ece9 to
d3fc9b9
Compare
d3fc9b9 to
f3ff5e1
Compare
|
@copilot review |
f3ff5e1 to
2fb66d4
Compare
IT-VBFK
changed the title
2fb66d4 to
51c3c6b
Compare
0550dd3 to
8e438a1
Compare
|
Do not merge, please (meaning now). I rebase the commits later to better commits :) |
fb768bd to
32902ef
Compare
|
@avidenic Can you do the final review.. mine isn't worth it (person from IT-VBFK == ITaluone) ;) |
|
As I'm cleaning up the migration, please wait with the merge after which I've taken a look. |
avidenic
left a comment
avidenic
left a comment
There was a problem hiding this comment.
Good improvements, some small issues to look at. Some should probably go into follow up issues / PRs. Or we can decide we're fine and just add docs about gaps.
80cc687 to
a3240c0
Compare
avidenic
left a comment
avidenic
left a comment
There was a problem hiding this comment.
Substantial improvements. There are still some decisions to be made though.
dennisdoomen
left a comment
dennisdoomen
left a comment
There was a problem hiding this comment.
I'd like to up our game here, even for our migration code, as that is going to be existing for years to come. So I'm going to be a bit more critical on the code review level. Don't get me wrong. We love all these contributions, but Nuke's code hasn't been particularly well factored and documented. So bear with me.
🔧 The PRs should explain what they are trying to do. Just a reference to an old (potentially convoluted) Nuke issue isn't enough anymore.
📝 The new code in this PR is IMO too long, too cryptic and has too little documentation.
|
Ok.. I propose to start over with small focused steps Ok @dennisdoomen? |
Yeah, and base it on #507 |
|
Cool I couldn't imagine how far your migration refactoring could go 😂😂 |
378eb2a to
02e6f48
Compare
02e6f48 to
75c7270
Compare
Closes #477