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

support netstandard2.0 #890

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

smoothdeveloper
Copy link

I'm not sure if the choice for netstandard2.1 only was explicit or just happened.

I can't consume any assemblies from .net framework and I think it hurts people trying to transition from older releases of this repository (that were or may remain .net framework only).

Since the changes seem minor, I hope this can be considered, if this doesn't hold undergoing progress.

Note that due to Path.GetRelative and the workaround I have being not 100% ideal, I've set the project using it to still has both netstandard2.0 and netstandard2.1 (as in, it shouldn't impact consumers of new versions that were anyways consuming the netstandard2.1 assemblies.

The build of the repository is nice, I had to deal a bit with fantomas and I think we should improve the message when --check is infringing, I'll see if I can bring something about in fantomas itself.

@smoothdeveloper
Copy link
Author

For context, I intend to consume this for https://ci.appveyor.com/project/smoothdeveloper/fsharp-data-sqlclient-an46a/builds/48667306.

If I put effort in upgrading, and given the state and constraints of the FSharp.Data.SqlClient repository, I need to be super conservative in adjusting each piece, I'm not super familiar with fsdocs and intend first to fix the branch where I move from SDK v2, the docs is the last step I need to fix for this and I'd prefer to do the minimum adjustments in order to merge the update to SDK 7, before I can. consider spending time on refreshing the docs for the project.

I also do intend to look at consuming this for .net framework project, right now I can't use the project for work solution due to netstandard2.1 only.

@nhirschey
Copy link
Collaborator

I’m not sure the reason for netstandard2.1. It seemed to happen in PR #621.

@smoothdeveloper
Copy link
Author

Thanks @nhirschey, after a quick review, @dsyme there is putting considerations about FSharp.Formatting consumers, and more generally FCS and other things in F# tooling, with regard to not forcing everyone to update (to latest FSharp.Core, and then the rest).

So I'm hopeful this can be considered for inclusion, I can probably nudge / polish it more if we are concerned about something getting suddenly broken with this PR.

Copy link
Collaborator

@nojaf nojaf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR introduces 13 new problems detected by the analyzers.
Please run dotnet fsi build.fsx -- -p Verify and fix these.

Your ongoing comments about Fantomas in multiple repositories are unnecessary and unproductive. Please discontinue this practice to maintain a focused environment.

Regarding the proposed change, I remain undecided. Without a clear understanding of your specific use case, I am not inclined to support this addition. In the past three years, there has been no similar request, raising questions about its necessity. Are you directly utilizing these NuGet packages in a scenario where you have to run this in the old runtime?

@nhirschey
Copy link
Collaborator

@smoothdeveloper, FSharp.Data is netstandard2.0 and it builds docs using the latest version of FSharp.Formatting.

Based on that I’m not sure this PR is necessary to upgrade FSharp.Data.SqlClient to modern docs.

@nhirschey
Copy link
Collaborator

@smoothdeveloper looks like you’re coming from scaffold, see here for possibly slightly out of date guidance: https://fsprojects.github.io/FSharp.Formatting/upgrade.html

@smoothdeveloper
Copy link
Author

@nojaf, thanks.

I'll give a check to the analyzers, I also thought of two changes:

  • instead of comment in the namespace for importing Utils, I'd wrap with the same precompiler, and only import the module for netstandard2.0
  • adding both targets to all projects, in case people consume assemblies with paths to them (I know it is not the main use case for the tool and code here)

Your ongoing comments about Fantomas in multiple repositories are unnecessary and unproductive. Please discontinue this practice to maintain a focused environment.

Please give yourself some distance or ignore them if they affect you, they are both meant light heartedly in case people take me too seriously, and they must echo something I'm only the catalyst for, in some manner, at least give it some slack.

I'm personally looking for using it in places where I know it will help, but I likely need more knobs (reverse of .fantomasignore, etc.), and some of the views, however may clash with choices, just expand perspectives, if we aren't emotionally affected.

I also think framing formatting discussion with preparing list of options upfront, and which are those options in the samples given is a very productive thing, this came through my comments, we can take "only the good parts" and ignore the rest.

So sorry, but we have to tolerate me some more, I'll keep in mind you didn't like it and respect your feedback, which also must echo some from others.

In the past three years, there has been no similar request

I'd rather look at how many projects have to yet upgrade, and how easy it is for those, when done by maintainers (not by Don or you who must have done it a lot), I thought in my case, to complete my upgrade to sdk 7 and allow me to consume the bits in more general context, netstandard2.0 is good option, doesn't hurt.

Comments from Don in #621 also highlight we shouldn't upgrade depedencies at high pace for core projects, due to chain of dependencies.

I think this holds, even if most people just consume fsdocs as a tool.

Additional points:

  • I'm likely to need to consume the assemblies, in "all .net framework" repository (but I still have the dotnet sdk, just can't use the runtime but in few places), and look at generating standalone html and leverage some features at a lower level, not what fsdocs is most geared towards (website for F# projects)
  • I also think .net ecosystem should avoid taking netstandard2.1 as lowest target, unless there is a strong necessity
  • I'll manage if we can't merge / ship this
  • I intend to use the tool at first in regular use case, but not for internal usage at work (will need to consume lower level, probably in process that loads .net framework assemblies), and maybe not initially in FSharp.Data.SqlClient sdk 7 upgrade PR

@nhirschey, thanks, exactly as first thing, I'm upgrading from scaffold, and also need to check more this guide, and maybe things will be smooth for FSharp.Data.SqlClient.

You've probably seen I made similar changes in few repository, probably because new projects are initialized with netstandard2.1, but overall, it doesn't hurt but few cases, to support netstandard2.0 as lowest target, otherwise it cuts the ecosystem in two.

For FSharp.Data.SqlClient, it takes a lot of time to adjust things for the CI, and updating dotnet sdk, I wished to complete the migration without having to do the overhaul of the docs but still making one step to consume the fresh dependencies, so I'm closer to have the good bits running next time I put efforts.

I'll check more if I can consume netstandard2.1 assemblies for the upgrade or complete the guide.

Thanks for the reviews everyone!

@nhirschey
Copy link
Collaborator

Just realized I also maintain a netstandard2.0 library (FSharp.Data.Toolbox) that I converted from scaffold to modern fsdocs. Definitely doable.

Anyway, thinking back on my experience going from scaffold to fsdocs, I'd avoid trying to "upgrade" the scaffold docs build pipeline as much as possible. Just take your content from "docs/content" and follow the zero-to-hero guide for a clean docs build. I think starting over, porting only the content rather than your full docs build pipeline, will be easier.

@smoothdeveloper
Copy link
Author

Thanks @nhirschey, I'm hopeful and will give it another shot.

Also, not a big deal if we can't get this PR merged, I understand why it could be even without knowing details, it was also a way to try out the repository for myself, it is great!

I think it is good to keep a door open to more direct usage of lower bits, AFAIU fsdocs tool isn't going to help if it is only entry point, for specific use case (snippets, integrate F# bits in other documentation system like sandcastle, and others), and I believe the community using the lower bits won't make a mess of this repository if we keep the door open, just expand the capabilities, more pretty printed (and formatted) F# everywhere 🙂.

@smoothdeveloper
Copy link
Author

I adjusted for analyzers, although in case it is something specific to this repository or my own environment, the output contains this:

MSBuild version 17.8.3+195e7f5a3 for .NET
unhandled expression at unhandled expression at C:\dev\src\github\fsprojects\FSharp.Formatting\src\fsdocs-tool\Program.fs:(10,4--16,9)
unhandled expression at C:\dev\src\github\fsprojects\FSharp.Formatting\src\fsdocs-tool\Program.fs:(10,4--16,9)
unhandled expression at C:\dev\src\github\fsprojects\FSharp.Formatting\src\fsdocs-tool\Program.fs:(10,4--16,9)
unhandled expression at C:\dev\src\github\fsprojects\FSharp.Formatting\src\fsdocs-tool\Program.fs:(10,4--16,9)
unhandled expression at C:\dev\src\github\fsprojects\FSharp.Formatting\src\fsdocs-tool\Program.fs:(10,4--16,9)
unhandled expression at C:\dev\src\github\fsprojects\FSharp.Formatting\src\fsdocs-tool\Program.fs:(10,4--16,9)
unhandled expression at C:\dev\src\github\fsprojects\FSharp.Formatting\src\fsdocs-tool\Program.fs:(10,4--16,9)
unhandled expression at C:\dev\src\github\fsprojects\FSharp.Formatting\src\fsdocs-tool\Program.fs:(10,4--16,9)
C:\dev\src\github\fsprojects\FSharp.Formatting\src\fsdocs-tool\Program.fs:(10,4--16,9)
unhandled expression at C:\dev\src\github\fsprojects\FSharp.Formatting\src\fsdocs-tool\Program.fs:(10,4--16,9)
unhandled expression at C:\dev\src\github\fsprojects\FSharp.Formatting\src\Common\StringParsing.fs:(325,12--325,24)
unhandled expression at C:\dev\src\github\fsprojects\FSharp.Formatting\src\Common\Collections.fs:(58,8--64,23)
unhandled expression at C:\dev\src\github\fsprojects\FSharp.Formatting\src\Common\Collections.fs:(58,8--64,23)
unhandled expression at C:\dev\src\github\fsprojects\FSharp.Formatting\src\Common\StringParsing.fs:(325,12--325,24)
unhandled expression at C:\dev\src\github\fsprojects\FSharp.Formatting\src\Common\Collections.fs:(58,8--64,23)
unhandled expression at C:\dev\src\github\fsprojects\FSharp.Formatting\src\Common\StringParsing.fs:(325,12--325,24)
unhandled expression at C:\dev\src\github\fsprojects\FSharp.Formatting\src\Common\Collections.fs:(58,8--64,23)
unhandled expression at C:\dev\src\github\fsprojects\FSharp.Formatting\src\Common\StringParsing.fs:(325,12--325,24)
unhandled expression at C:\dev\src\github\fsprojects\FSharp.Formatting\src\Common\StringParsing.fs:(325,12--325,24)
unhandled expression at C:\dev\src\github\fsprojects\FSharp.Formatting\src\Common\Collections.fs:(58,8--64,23)
unhandled expression at C:\dev\src\github\fsprojects\FSharp.Formatting\src\FSharp.Formatting.Markdown\MarkdownParser.fs:(1170,4--1378,5)

I checked the code around those locations but didn't find anything suspect; along which I found this code and was wondering if it is tracked as fantomas issue or if we should try to minimize the reproduce for it and track it:

https://github.com/fsprojects/FSharp.Formatting/blame/6e1443941998de1f72d98c562624e3754f3dc6ae/src/FSharp.Formatting.Markdown/MarkdownParser.fs#L1153-L1159

As for this PR journey, if we don't find it appealing, I suggest to keep it open as a place where other people may discuss if they want netstandard2.0 support, and eventually close it when it is time to close "non merged / stalled PRs".

Since I'm not sure my feedback on fantomas is welcome, the question specific to this repository would be and how it is integrated:

can we run fantomas without --check if it is not a CI run?

Bonus is, can fantomas quote back the command line minus --check and say

you may run 'this' to fix the code formatting

(where this is whole quoted command without the --check)

If this is relevant to fantomas, and needed, I can bring an issue to fantomas repository.

@smoothdeveloper smoothdeveloper marked this pull request as draft December 9, 2023 00:45
@smoothdeveloper
Copy link
Author

Note that there is a subtletly with how the FSharp.Formatting nuget package is made, in order for this to work properly I think I need to make the layout of the .nupkg remain the same, and the .fsx to work with either netstandard2.0 and netstandard2.1 paths.

I'll look a bit more if / how this can be done.

Turned into a draft for now.

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