Skip to content

Provide API that includes printf specifier arities along with ranges #527

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

Merged
merged 6 commits into from
Feb 8, 2016

Conversation

latkin
Copy link
Member

@latkin latkin commented Feb 8, 2016

Motivation: fsprojects-archive/zzarchive-VisualFSharpPowerTools#1335

Tools that wish to map from specifier->argument(s) or vice versa need to know not only the range of the specifier, but also its arity. Most usages involve only 1 argument per specifier, but there are special cases with * width and/or precision slots, or the %a specifier, which add additional arguments.

Closely related - %%/%*%/%*.*% will now be reported as a format specifiers (with arity 0/1/2).

…ier along with its range

This is needed if one wants to accurately map arguments -> specifiers or
specifiers -> arguments.

A specifier can take 0, 1, 2, or 3 args:

  - printfn "%%"     // 0 args
  - printfn "%*%"    // 1 arg
  - printfn "%*.*%"  // 2 args
  - printfn "%*.*f"  // 3 args
@@ -262,6 +262,9 @@ type FSharpCheckFileResults =
/// <summary>Get the locations of format specifiers</summary>
member GetFormatSpecifierLocations : unit -> range[]
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add Obsolete to this, with a message to use the new method

@dsyme
Copy link
Contributor

dsyme commented Feb 8, 2016

Great work!

There's a test failure, suspect it won't be too hard to fix

[00:17:36] 1) Test Failure : FSharp.Compiler.Service.Tests.Editor.Printf specifiers for regular and verbatim strings
[00:17:36]      Expected: [||]
[00:17:36] Actual: [|/home/user/Test.fsx (33,45)-(33,47) typecheck error This expression was expected to have type
[00:17:36]     string    
[00:17:36] but here has type
[00:17:36]     unit    ;
[00:17:36]   /home/user/Test.fsx (34,49)-(34,51) typecheck error This expression was expected to have type
[00:17:36]     string    
[00:17:36] but here has type
[00:17:36]     unit    |]
[00:17:36]   Expected is <Microsoft.FSharp.Compiler.FSharpErrorInfo[0]>, actual is <Microsoft.FSharp.Compiler.FSharpErrorInfo[2]>
[00:17:36]   Values differ at index [0]
[00:17:36]   Extra:    < </home/user/Test.fsx (33,45)-(33,47) typecheck error This expression was expected to have type
[00:17:36]     string    
[00:17:36] but here has type
[00:17:36]     unit    >, </home/user/Test.fsx (34,49)-(34,51) typecheck error This expression was expected to have type
[00:17:36]     string    
[00:17:36] but here has type
[00:17:36]     unit    > >
[00:17:36] at FSharp.Compiler.Service.Tests.Editor.Printf specifiers for regular and v
[00:17:36] erbatim strings() in C:\projects\fsharp-compiler-service\tests\service\EditorTests.fs:line 407

@latkin
Copy link
Member Author

latkin commented Feb 8, 2016

Whoops, thanks. All should be good now.

dsyme added a commit that referenced this pull request Feb 8, 2016
Provide API that includes printf specifier arities along with ranges
@dsyme dsyme merged commit cbfe460 into fsharp:master Feb 8, 2016
@dsyme
Copy link
Contributor

dsyme commented Feb 9, 2016

@rneatherway Would you be able to push a new nuget package?

@latkin Would you like to have publish-the-package rights? There's no one better qualified :)

@latkin
Copy link
Member Author

latkin commented Feb 9, 2016

Sure, I'd be honored

@dsyme
Copy link
Contributor

dsyme commented Feb 9, 2016

@latkin OK you have invitations to Write access to this repo and to the two nuget package repos.

Think that's all you need to publish. I find the build.fsx publishing process can go wrong sometimes in different repos. It seems best to build+test the nupkg and push it manually, and then do GenerateDocs and ReleaseDocs carefully too. On occasion in different repos ReleaseDocs has nuked the whole contents of the github-pages branch, requiring a forced push or two.

@rneatherway
Copy link
Member

That is great if someone who uses Windows can do the releases. I never managed to create a single Windows installation where the tests passed and the SourceLink stuff worked.

@latkin
Copy link
Member Author

latkin commented Feb 9, 2016

FWIW tests don't pass on my machine, either.

@dsyme
Copy link
Contributor

dsyme commented Feb 9, 2016

Tests pass on mine, at least last time I checked. @latkin could you please paste in the errors you're getting?

I don't know what is going on with SourceLink and CRLF/LF. One repo copy on my machine has CRLF and the other has LF. SourceLink only agrees to build the nuget with one of them. I'll file a SourceLink bug.

@dsyme
Copy link
Contributor

dsyme commented Feb 9, 2016

Add the sourcelink issue here: ctaggart/SourceLink#119. Until then feel free to disable the SourceLink step if it fails while preparing a release.

@latkin
Copy link
Member Author

latkin commented Feb 9, 2016

ok, I'll clone/build/test clean later on and open an issue with failures.

@rneatherway
Copy link
Member

If I use git clone https://github.com/fsharp/FSharp.Compiler.Service -c core.autocrlf=input then sourcelink is OK on my VS2013 machine. But on that machine some tests fail. On my VS2015 VM I can't work out what Windows Debugging Tools package to install from the MS website. When I release I disable one or both of those steps, but it isn't ideal.

@rneatherway
Copy link
Member

I have a tiny PR at #528 that gives me a clean run-through of both the tests and SourceLink. This VM has VS2015 and the Windows 10 debugging tools installed.

@ctaggart
Copy link
Contributor

Guys, if you publish the nupkg files built by AppVeyor, you can just run SourceLink target there by adjusting the build.fsx. Change the line to =?> ("SourceLink", isAppVeyorBuild).

@rneatherway
Copy link
Member

Guys, if you publish the nupkg files built by AppVeyor

That sounds great, is it easy to promote them?

@ctaggart
Copy link
Contributor

Very easy. Download them from the AppVeyor project NuGet feed and upload them to the NuGet Gallery with nuget.exe or paket.exe or add a new deployment if you are an admin for the project.
image

@rneatherway
Copy link
Member

I've released 2.0.0.5 which includes this change. I can now do a full build Release on one machine as long as I set nugetkey=XXX... first.

@latkin latkin mentioned this pull request Feb 16, 2016
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.

4 participants