TAGC / dotnet-setversion

.NET Core CLI tool to update the version information in .NET Core *.csproj files
MIT License
75 stars 16 forks source link

Implement support for specifying csproj file and --recursive option #10

Closed Skulblaka closed 6 years ago

Skulblaka commented 6 years ago

Things have gotten a little bit out of hand today and I ended up implementing the changes we discussed.

For input parsing, I'm using commandlineparser/commandline rather than doing it myself as I was already familiar with this library.

I also wrote some tests to cover the different usages of the cli.

TAGC commented 6 years ago

The check will probably take some time because for some reason AppVeyor is stuck on a commit I pushed (contacted support about this, will give it a bit of time before I cancel the build).

Skulblaka commented 6 years ago

Still curious about the params [...]. Not ever seen that five before.

Using params for Program.Main or in general?
It's just a convenience feature of C#, it allows a method to take a variable number of arguments.

I can remove it if it bothers you ☺️

TAGC commented 6 years ago

Using params for Program.Main or in general?

@Skulblaka for Program.Main specifically. Is there a benefit for its use there?

Skulblaka commented 6 years ago

@TAGC Not for regular usage (that is invoking from a shell). The only benefit (I know) of the params keyword in this case is that I can invoke Program.Main like Program.Main(version, csprojFile) rather than Program.Main(new[] {version, csprojFile}) from my tests.

TAGC commented 6 years ago

@Skulblaka Ah okay. I think it's justified to keep it in then.

TAGC commented 6 years ago

@Skulblaka Sorry for the inactivity on this. I'd like to merge but #11 is blocking things. AppVeyor seem to have changed something with their build servers sometime in the past 5 months that causes the program to hang.

I'm going on holiday on Thursday for a couple of weeks so I won't really be able to look into this myself. If you have any interest in looking into it yourself, the develop-keep branch updates the AppVeyor build script with a step that lets you RDP into the build server when builds are running (edit: although the CI build itself is currently only configured to trigger for commits on master or develop).

TAGC commented 6 years ago

@Skulblaka Hi, the CI build issue is fixed now. If you're still interested, can you rebase this PR on the latest version of my develop branch?

Skulblaka commented 6 years ago

@TAGC I'm not sure I did as you intended, but here you go. Is the AppVeyor build supposed to to succeed..?

TAGC commented 6 years ago

You merged rather than rebased. The end result is the same but the git history will be cleaner if you rebase.

It didn't hang which is the important thing. The reason it seems to have failed is because you've introduced dependencies to the tool (specifically CommandLineParser) which it can't resolve during e2e test package restores.

This is because I clear all package sources and only use a local feed in the e2e tests:

<?xml version="1.0" encoding="utf-8"?>
<configuration>
  <packageSources>
    <clear />
    <add key="tool-packages" value="out" />
  </packageSources>
</configuration>

I did this to be certain that only the locally-built distributions of dotnet-setversion would be tested. According to the NuGet docs, it should be safe to remove the <clear /> - it'll still use the locally-built versions of dotnet-setversion but also be able to pull CommandLineParser from the official NuGet feed.

TAGC commented 6 years ago

If you want, I also hang out in the C# Discord server where we could discuss this in real time.

Skulblaka commented 6 years ago

I don't like group chats, let alone one with so many people, but feel free to pm me: Skulblaka#9409

TAGC commented 6 years ago

@Skulblaka I got the build to work for all the Docker tests. I broke it again (deliberately) by including the integration tests you wrote. I think you just need to update the error codes in those tests, since the code now returns 1 for any sort of failure, but the tests expect different error codes. Once the integration tests are fixed, the whole build should pass.

Skulblaka commented 6 years ago

I fixed the tests by changing them to expect any non-zero exit code on failure.