Features/thanh/net 8 #400
Features/thanh/net 8 #400
Conversation
- Centrally nuget package - Add rule check
|
Thanks a lot for your PR, @hoangthanh28 ! This looks very good to me. I agree that using Directory.Build.props is a good idea, to avoid having to manage the nuget package versions in every .csproj (even though they are in the same solution). And, I think the IDE support for Directory.Build.props is widespread enough now as well, so I don't see any reason not to start using it :) And, of course, upgrading to stable .net 8 has been on my todo-list since it was released, I just haven't got around to it yet, so I appreciate your contribution! Also, a very good idea to add a Codeanalysis.ruleset file. That's a good start! And, if we find out that we want to change any of the rules going forward, it can always be adjusted later. I had a couple of questions/comments, could you please look through them? Otherwise, I'm good to merge this! |
| InternalBatchCommands = npgsqlCommand.GetProperty("InternalBatchCommands", BindingFlags.Instance | BindingFlags.NonPublic)!; | ||
| State = npgsqlCommand.GetProperty("State", BindingFlags.Instance | BindingFlags.NonPublic)!; | ||
| IsCached = npgsqlCommand.GetProperty("IsCached", BindingFlags.Instance | BindingFlags.NonPublic)!; | ||
| IsCached = npgsqlCommand.GetProperty("IsCacheable", BindingFlags.Instance | BindingFlags.NonPublic)!; |
There was a problem hiding this comment.
I know this is an ugly class, sorry :/
But, has the method changed name in the npgsql library, is that why you have changed from "IsCached" to "IsCacheable"?
There was a problem hiding this comment.
Yes, it has (I double-checked myself :) )
| @@ -2,36 +2,23 @@ | |||
|
|
|||
| <PropertyGroup> | |||
| <OutputType>Exe</OutputType> | |||
There was a problem hiding this comment.
Are these setting the $(TargetFramework) necessary, or will setting them in the Directory.Build.props directly work as well, and just removing them from the .csproj files?
There was a problem hiding this comment.
Yes, you can set them in the Directory.Build.props once and it will be inherited.
| @@ -64,7 +63,7 @@ public async Task Does_not_create_versions_on_DryRun() | |||
| await using var migrator = Context.GetMigrator(grateConfig); | |||
| await migrator.Migrate(); // shouldn't touch anything because of --dryrun | |||
| var addedTable = await migrator.DbMigrator.Database.VersionTableExists(); | |||
| Assert.False(addedTable); // we didn't even add the grate infrastructure | |||
| addedTable.Should().Be(false); // we didn't even add the grate infrastructure | |||
There was a problem hiding this comment.
I agree, I like fluent assertions better too ;)
Hi Erik.
This PR is mainly about upgrading the .net version to latest lts of .net (.net 8).
I'm not a big fan of floating package version, so please consider this PR as refactor it using central package management
Adding some rules check to improve the quality of the code.
Side note: I'm using VS code, the code format a little bit different than your, if you want to reformat using Rider, do let me know, happy to change it.