-
-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WIP: Remove parser, port C# Task, and other modernisation #30
Conversation
Oh, and I know 17 mentions making it C# deliberately: I figure we can always swap back if the repo gets closer to a model example with all the other best practice changes made :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a number of comments - this is all great work! The big question to me is what is the layout of the generated nupkg from this set of changes, and how does it compare to previous versions?
Swap to license expression Enable source link repo url
@baronfel I believe I've addressed all the current comments. One outstanding question I have: |
we should totally move to .NET 8 as part of this |
Enhancement that would make the diffs easier: If you add |
I'll re-apply your deduplication of the build props/target files as well |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks lovely - thank you for doing this. Do you have any final concerns? It all looks solid to me.
I think this is good as a first set: I'll look at improving the tests in the next PR! |
Ironically, I didn't update the CHANGELOG.md |
WHAT
This combines #17 and https://github.com/MangelMaxime/KeepAChangelog/tree/feature/rework
It removes the parser library, replacing it with an F# port of #17's C# implementation.
It adds the tests from @MangelMaxime 's work, as well as porting the test from #17.
It makes the general build changes from @MangelMaxime 's work, including central package management, and package lock. These are slightly amended to fix a few issues/redundant bits.
This now works in Visual studio, for both C# and F# SDK projects, for both net472 and net6/8.
WHAT NOT
This doesn't (yet) have enough tests.
This doesn't actually make any improvements to the features of the task.
WHY
I wanted to work on an msbuild task, and @MangelMaxime suggested this one.
If this is reasonable as an approach, I'd like to start adding some of the low hanging fruit in issues.