Skip to content

Target upgrade + update nuget#148

Closed
gabriel-vanca wants to merge 12 commits intocoenm:mainfrom
gabriel-vanca:retarget
Closed

Target upgrade + update nuget#148
gabriel-vanca wants to merge 12 commits intocoenm:mainfrom
gabriel-vanca:retarget

Conversation

@gabriel-vanca
Copy link
Copy Markdown
Contributor

Changed target so that systems stop complaining about not having some ancient version of .Net installed
Also updated nuget packages.

@coenm
Copy link
Copy Markdown
Owner

coenm commented Sep 5, 2024

Thanks for your effort to create a PR.

I'm willing to take some time to review the changes. However, I notice the build fails because you changed the global.json file.

@gabriel-vanca
Copy link
Copy Markdown
Contributor Author

Oh, interesting, it's not failing for me in Visual Studio. I did comment it out after forking as it does complain about the SDK version if I leave it uncommented, as I have a newer SDK version installed. I think the test might just be failing because comments aren't supposed to exist in JSON files. I removed it now. That seems to have fixed that problem, but now it's complaining the Windows target version is too new. Feel free to set it to 10.0.22621.0. My builder doesn't have either of these issues :(

@gabriel-vanca
Copy link
Copy Markdown
Contributor Author

Also, just a quick note about the NULL check at gabriel-vanca@589f7b6

That check should be unnecessary as the object technically cannot be null. Even the IDE is insisting I don't need the check because the object cannot be null. Yet, in at least one instance, that object was indeed null and running Stop() on it was crashing the entire app.

@gabriel-vanca
Copy link
Copy Markdown
Contributor Author

Fixed the build issues now.

@coenm coenm self-assigned this Sep 6, 2024
@coenm
Copy link
Copy Markdown
Owner

coenm commented Sep 6, 2024

I notice you addressed a number of things,

  1. The null check in DefaultRepositoryMonitor
    Thank you for finding this. Although I believe the overall problem is the non-thread-safety of the class, this might help. I would have accepted the PR with only this change in a sec.

  2. Changing behavior when compiled in Debug (#if DEBUG) mainly to disable functionality (UpdateMenuSize(string resolution, MenuSize size)), and to use other files for configuration (appsettings.debug.json) and storage ( Path.Combine(AppDataPathProvider.AppDataPath, "Repositories.cache.debug");). I recognize the problem you are trying to solve but I don't like the chosen implementation. I've created an issue (Use different configuration and settings when developing #153) for this problem such that we can discuss how to solve the issue first.

  3. Styling
    You updated the editorconfig and fixed a number of stylings throughout the code. I'm aware of the inconsistency of the code formatting and styling but this doesn't fix it. It seems as a partial update, and I also don't know if the editorconfig is what I would like. I have created an issue for this (Code style consistency #154) and I willing to discuss some formatting and how to address all code styling issues at once in a separate PR.

  4. Package updating
    Some of the used packages in RepoM I can upgrade almost without building and trying but for a few I had some problems with in the past. Therefore, I'm keen on updating packages and I find it 'hard' to accept this PR. Also, the Nerdbank.GitVersion package is defined in a "Directory.Build.props" and should only be updated in that file.

  5. Misc
    Specifying SupportedOSPlatformVersion like in

  <PropertyGroup>
    <TargetFramework>net8.0-windows10.0.26100.0</TargetFramework>
    <SupportedOSPlatformVersion>10.0.26100.0</SupportedOSPlatformVersion>
  </PropertyGroup>

Thanks, I'm not quite sure what this does but I would really like to find out. I've created an issue (#155) to do some research. For now, I don't merge it as I don't know what it means.

Again, thanks for your effort. If you revert all changes except the null check I will gladly accept your PR.

@gabriel-vanca
Copy link
Copy Markdown
Contributor Author

Thank you for your helpful feedback, Coen. I recognise this PR was a bit of a mess as I was just getting used to the code. I think it's best to close it and I'll open a new one. Just a few notes on what you said:

  1. I'll get right on it.
  2. I tried to generate a special Debug folder initially, but that generated a myriad of errors. Generating .debug files is the only fix I could find after trying for about 90 minutes. Also tried to create a special log file, but that is defined in many places, some of which don't support #if DEBUG. It's highly frustrating to debug while I have RepoM installed as well. Best to continue the discussion on Use different configuration and settings when developing #153 though.
  3. The editorconfig I suggested is functionally the same as the original, just clarifying some extensions. I'm ok with keeping the original though, especially if you're thinking about using something else. Let's continue that discussion on Code style consistency #154.
  4. I'm guessing you had issues with the Material package update. Yes, that requires some changes in App.xaml and MainWindow.xaml to use the new Material 3 Themes. I already have a commit prepared for that, but I'll make it in a separate PR from the rest as it leads to a (slight-ish) visual change as well. I will not update the other packages for now either, albeit I had no issue with them.
  5. The default for .NET8 is targeting Windows 7. (https://learn.microsoft.com/en-us/dotnet/standard/frameworks). I think best would be targeting Windows 11 and adding support for 10. I suggested targeting Windows 11 22H2 (10.0.22621) and support for Windows 11 21H2 (10.0.22000) or even Windows 10 20H1 (10.0.19041). I doubt anyone running RepoM will be on older systems than that :) We can continue the conversation at Support older OS versions #155 as you suggested and do more research. Or just wait for .NET9 later this year (I think?) and see then.

@gabriel-vanca
Copy link
Copy Markdown
Contributor Author

See #159

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.

2 participants