#1611 Project file updating

[svengeance]

Fixes #1611

This PR allows the ability to update csproj/vbproj/fsproj files directly, rather than forcing users to implement <GenerateAssemblyInformation>false</GenerateAssemblyInformation> in their project files.

Description

I tried to keep my programming in line with how the AssemblyVersionInfoUpdater performs its tasks, so there should be no surprises as to how this works. I first acquire all project files explicitly named or discovered, verify they are in a format that I am comfortable parsing, and then simply add the version elements.

Libraries Used

I opted to go with Linq2Xml over incorporating Microsoft.Build for a couple reasons; I know you guys are approaching a new rearch, and I wanted to ensure my changes didn't incorporate something large, and I also had issues out-of-the-box with the package, and was uncomfortable shipping that.

Update Logic

There are quite a few scenarios and odds-and-ends with versioning in the project files. Users can have multiple property groups and have multiple elements spread throughout (i.e. they can have two <AssemblyFileVersion>s in their project, where the last-in wins. To account for this, my implementation seeks the last <PropertyGroup> that has a version element and updates it, or updates the first available <PropertyGroup> to have the version element.

CLI Usage

I opted to hijack the usage of /updateassemblyinfofiles as it was one of the least-invasive opportuinities to enter. Project file updating is triggered by adding another switch /targetprojectfiles. I'm totally open to suggestions on this.

Testing

As far as testing goes I've opted to use naked XML in the test cases (and sometimes for the expected value) as I believe it offers the best visibility possible into the before-and-after changes. Note, I am missing some full integration tests (e.x. create a file, call Program in that directory), but I'm unsure what scope of test you guys want.

Misc

I updated IFileSystem to Enumerate Files rather than returning the list. We were using them in contexts of IEnumerable and yielding results, so it'll prevent the big ol' spike of loading files and should instead parse them one at a time.

Related Issue

#1611

Motivation and Context

By default .NET Core applications do not have an AssemblyInfo.cs. This change allows us to support out-of-the-box versioning without customers having to make any changes.

How Has This Been Tested?

The core functionality can be broken down into the overall integration with two subcomponents.

• Update/replace file
  • Determine if we can update file
  • Update XML structure

I've unit tested the two pieces of business logic, and I have one test following some prior tests that I believe tests the integration of the whole.

Screenshots (if appropriate):

None, but here's a picture of one of my cats for your pleasure: https://media.discordapp.net/attachments/631864419693101056/694290169691373668/20200330_170018.png?width=647&height=862

Checklist:

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• All new and existing tests passed.

[asbjornu]

There's 2 failing tests and I have a suggestion for the name of the argument, otherwise this looks golden.

[svengeance]

@asbjornu I've made another commit to rename the switch. I suppose the only concern is that a user may assume that that switch is independently capable of updating, but you do need both /updateassemblyinfo and /updateprojectfiles

Regarding the failing tasks - I'm not sure how to resolve those. The errors seemed to be out of my control, but let's wait for this latest build

[asbjornu]

Why is updateprojectfiles dependent on updateassemblyinfo? Aren't they independent from one another?

Regarding the failing tasks - I'm not sure how to resolve those. The errors seemed to be out of my control, but let's wait for this latest build

All tests pass now, so we're good. 😃

[svengeance]

I mentioned that in my CLI section --

I opted to hijack the usage of /updateassemblyinfofiles as it was one of the least-invasive opportuinities to enter. Project file updating is triggered by adding another switch /targetprojectfiles. I'm totally open to suggestions on this.

I can add additional logic to this section if need be to make it independent. I was having a difficult time parsing through the arg-parsing logic and wanted to keep my changes minimal there

[svengeance]

I guess -- put another way, updating AssemblyInfo compared to updating ProjectFiles differs only in 2 aspects: /ensureassemblyinfo is invalid for ProjectFiles, and we target .csproj files instead of .cs files. Given that the logic is majorly the same I thought it reasonable for this to be a modifier of the action rather than a new action unto itself.

[asbjornu]

While I understand your reasoning, aren't the two options mutually exclusive anyway?

[svengeance]

True enough. Like I said I'm totally open to changing how it's used. Give me a bit and I'll rewire it to be its own action.

[asbjornu]

Great, @svengeance! While you're at it, will you please perform a rebase against the HEAD of master as well? 🙏 😃

[svengeance]

@asbjornu Updated. I've left assembly/project updating loosely related in that they're in the same set of configurable options, but you can now independently target one or the other through the CLI. I imagine that the transition to System.CommandLine will mimic this approach, where you have a base command for updating files that all /updatexyz shares which will bind to a base set of options like the target files.

I've also explicitly disallowed updating both project and assemblyinfo files in the same run, but theoretically this should be able to be done by removing the WarningExceptions and changing the else if in GitVersionTool.cs to an if.

*I should add that if you're not happy with this approach I can go for a complete separation wherein project files have their own set of options and everything, I just didn't feel like it had to go that far.

[svengeance]

Oh, and I've rebased onto HEAD as requested.

[asbjornu]

Besides the breaking change, I think this looks good.

[svengeance]

@asbjornu What else would you like to see on this? Personally I'd love some sort of full integration test starting from the top of Program.cs but I'm not sure if that's feasible. I plan on adding at least one or two ArgParser tests, but other than that, what else?

[asbjornu]

A couple of argument parser tests would be good. Otherwise, I think the functionality itself is well enough tested. As this is a new feature no one uses yet, we can be more lenient on the testing and allow the community to test a bit for us. Then we add regression tests as needed if bugs pop up.

[svengeance]

@asbjornu I think this may be ready. I've added a few parsing tests, but given their incoming demise, I don't see much value in being thorough. Let me know if/what else you'd like to see here 🎉

[asbjornu]

Excellent, @svengeance! Thank you so much for your contributions. 🙏

[github-actions]

🎉 This issue has been resolved in version 5.3.4 🎉
The release is available on:

• GitHub Release
• Chocolatey Package
• .Net Global Tool

Your GitReleaseManager bot 📦🚀