Skip to content

Comments

WIP: Remove parser, port C# Task, and other modernisation#30

Merged
baronfel merged 25 commits intoionide:mainfrom
tboby:fix-rework
Oct 22, 2024
Merged

WIP: Remove parser, port C# Task, and other modernisation#30
baronfel merged 25 commits intoionide:mainfrom
tboby:fix-rework

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
Contributor

@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?

@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
Contributor

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

@baronfel
Copy link
Contributor

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
Contributor

@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
@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
@goswinr
Copy link

goswinr commented Mar 18, 2025

@baronfel could you publish a new NuGet? Maybe prerelease? Users of my libraries (that have this task) also reported the „Fparsec bug“

@goswinr
Copy link

goswinr commented Aug 18, 2025

@tpetricek @Krzysztof-Cieslak @nojaf @baronfel I am not sure who maintains this repo. Since this PR is merged, could any of you publish a new nuget ?

@nojaf
Copy link
Contributor

nojaf commented Aug 18, 2025

I don't have any permissions here, I believe only @baronfel does.

@TheAngryByrd
Copy link
Member

Sorry about that. I pushed 0.3.0 today.

@goswinr
Copy link

goswinr commented Sep 8, 2025

Thanks @TheAngryByrd . Similar to #37, I also had a few CI builds fail.

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.

5 participants