dotnet / fsharp

The F# compiler, F# core library, F# language service, and F# tooling integration for Visual Studio
https://dotnet.microsoft.com/languages/fsharp
MIT License
3.83k stars 773 forks source link

Allow ParsedHashDirectives to take non string arguments #17206

Closed KevinRansom closed 2 weeks ago

KevinRansom commented 1 month ago

Requires fslang approval : https://github.com/fsharp/fslang-suggestions/issues/1368

Fixes: #17209 , #17240

github-actions[bot] commented 1 month ago

:heavy_exclamation_mark: Release notes required


:white_check_mark: Found changes and release notes in following paths:

[!WARNING] No PR link found in some release notes, please consider adding it.

Change path Release notes path Description
src/Compiler docs/release-notes/.FSharp.Compiler.Service/8.0.400.md No current pull request URL (https://github.com/dotnet/fsharp/pull/17206) found, please consider adding it
LanguageFeatures.fsi docs/release-notes/.Language/preview.md No current pull request URL (https://github.com/dotnet/fsharp/pull/17206) found, please consider adding it
abelbraaksma commented 1 month ago

Fixes: #17209

did you (also) mean #17240? It is mentioned in the preview.md release notes. And this closed suggestion: https://github.com/fsharp/fslang-suggestions/issues/1364.

Considering this is a language change now (if I understand it correctly #nowarn "12" and #nowarn 12 are now both allowed, same for #time "on" and #time on), I propose we (I mean: I don't mind doing it) write an RFC for this. I don't mean to slow-down progress, but otherwise we won't catch this in the F# spec effort.

abelbraaksma commented 1 month ago

Considering the long ident change, does this mean #r "System.Core" and #r System.Core are now both allowed?

EDIT: checking your referenced issues, the answer is: yes 👍. I've tried to capture all these changes here: https://github.com/fsharp/fslang-suggestions/issues/1368.

KevinRansom commented 1 month ago

Considering the long ident change, does this mean #r "System.Core" and #r System.Core are now both allowed?

Sigh!!!!, I didn't intend for that to work.
#I and #r and #r nuget require :, \ and / which means we would have to teach the parser paths and URIs.

KevinRansom commented 1 month ago

@abelbraaksma huge thanks for taking care of the fslang suggestion, I miss having an active PM to keep me on the true path of righteousness.

abelbraaksma commented 1 month ago

I miss having an active PM to keep me on the true path of righteousness.

You're too kind!!! Are you offering me job? 😆jk. But seriously, no worries, I'm happy to help :).

require :, \ and / which means we would have to teach the parser paths and URIs.

Not entirely. It could just be that simple paths that are in scope are allowed without quotes, anything else needs quotes. Just like you don't need quotes on the commandline, unless you have spaces in the file names.

But we can decide that in the lang suggestion, of course.

Another way for this change is that you keep the information after you parse it. That way, it is not that all hash directives can now understand all kinds of arguments. That way, you can pick and choose per directive.

edgarfgp commented 3 weeks ago

Now with https://github.com/dotnet/fsharp/pull/17140 merged. Looking forward to have this in 👍🏻

psfinaki commented 3 weeks ago

/azp run

azure-pipelines[bot] commented 3 weeks ago
Azure Pipelines successfully started running 2 pipeline(s).
nojaf commented 2 weeks ago

Nitpick: this should have had some tests in https://github.com/dotnet/fsharp/tree/main/tests/service/data/SyntaxTree/ParsedHashDirective