Skip to content
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

Merged
merged 25 commits into from
Oct 22, 2024

Conversation

tboby
Copy link
Contributor

@tboby tboby commented Oct 21, 2024

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.

@tboby
Copy link
Contributor Author

tboby commented Oct 21, 2024

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 :)

Copy link
Collaborator

@baronfel baronfel left a 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?

Directory.Build.props Outdated Show resolved Hide resolved
Directory.Build.props Outdated Show resolved Hide resolved
Directory.Build.props Outdated Show resolved Hide resolved
Directory.Packages.props Outdated Show resolved Hide resolved
global.json Outdated Show resolved Hide resolved
@tboby
Copy link
Contributor Author

tboby commented Oct 22, 2024

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?
2lrwew7QLX

@tboby
Copy link
Contributor Author

tboby commented Oct 22, 2024

@baronfel I believe I've addressed all the current comments.

One outstanding question I have:
Is there any benefit to targeting net6.0 in the project if we require net8.0 msbuild?

@baronfel
Copy link
Collaborator

we should totally move to .NET 8 as part of this

@baronfel
Copy link
Collaborator

Enhancement that would make the diffs easier:

If you add <SatelliteResourceLanguages>en</SatelliteResourceLanguages> as a property to the Directory.Build.props all of those different language translations from FSharp.Core won't flow through to the packages.

@tboby
Copy link
Contributor Author

tboby commented Oct 22, 2024

I'll re-apply your deduplication of the build props/target files as well

@tboby
Copy link
Contributor Author

tboby commented Oct 22, 2024

image

baronfel
baronfel previously approved these changes Oct 22, 2024
Copy link
Collaborator

@baronfel baronfel left a 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.

@tboby
Copy link
Contributor Author

tboby commented Oct 22, 2024

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!

@baronfel baronfel merged commit 8fd6473 into ionide:main Oct 22, 2024
1 check passed
@tboby
Copy link
Contributor Author

tboby commented Oct 22, 2024

Ironically, I didn't update the CHANGELOG.md

@tboby tboby deleted the fix-rework branch October 22, 2024 21:27
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.

3 participants