Skip to content

Support putting two format specifiers next to each other #620

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 33 commits into from
Jun 14, 2025

Conversation

Happypig375
Copy link
Contributor

@Happypig375 Happypig375 commented May 29, 2025

Right now, trySscanf on e.g. "%c%c" won't work because all specifiers are translated to (.*?) under the hood.

Also added:

  • format specifier consumes spaces to left
  • - format specifier consumes spaces to right
  • + format specifier consumes preceding plus for numbers
  • (|Scan|_|) that turns trySscanf into an active pattern

@gusty
Copy link
Member

gusty commented May 29, 2025

Indeed

> trySscanf "%c%c" "ab" ;;
val it: (char * char) option = None

But I was expecting Some ('a','b').
Hopefully you can fix it.

@Happypig375 Happypig375 marked this pull request as draft June 10, 2025 14:35
@Happypig375 Happypig375 marked this pull request as ready for review June 10, 2025 17:04
@Happypig375
Copy link
Contributor Author

Test failure seems... unrelated?

@gusty
Copy link
Member

gusty commented Jun 10, 2025

Not sure, at first sight it looks related, here's what I am seeing:

           Failed scanfParsing [62 ms]
           Error Message:
            System.IndexOutOfRangeException : Index was outside the bounds of the array.
           Stack Trace:
              at FSharpPlus.Tests.Parsing.scanfParsing() in C:\a\FSharpPlus\FSharpPlus\tests\FSharpPlus.Tests\Parsing.fs:line 87
            at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
            at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)

Have you tried to repro locally ?

I normally paste code fragments in a script to work it out. It's way quicker than compiling the whole lib and trying stuff.

@Happypig375
Copy link
Contributor Author

When I try to debug locally, Visual Studio just hangs on me...

@gusty
Copy link
Member

gusty commented Jun 11, 2025

That's why I always use (and suggest) a script.
Loading the whole solution is not a practical approach at all.

@Happypig375
Copy link
Contributor Author

Ok, so the root cause is that sometimes we do want to consume spaces before and after as shown in one of the existing tests. I've added support via the and - modifiers - consumers spaces before and - consumes spaces after.

@Happypig375
Copy link
Contributor Author

I think it's now fixed

@Happypig375
Copy link
Contributor Author

I rewrote it to be more performant and also removed the spaces

let inline trySscanf (pf: PrintfFormat<_,_,_,_,'``(T1 * T2 * ... * Tn)``>) : string -> '``(T1 * T2 * ... * Tn)`` option = getGroups pf >> TryParseArray.Invoke

/// Matches a formatted text with the result of parsing each element. Will not match in case of failure.
let inline (|Scan|_|) (pf: PrintfFormat<_,_,_,_,'``(T1 * T2 * ... * Tn)``>) : string -> '``(T1 * T2 * ... * Tn)`` option = trySscanf pf
Copy link
Member

Choose a reason for hiding this comment

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

Does it makes sense to re-use our existing Parsed active pattern ?

It think this one expand it, but it doesn't break it.

Copy link
Contributor Author

@Happypig375 Happypig375 Jun 13, 2025

Choose a reason for hiding this comment

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

How can you fit a format string into (|Parsed|_|): string -> 'T option?

Copy link
Member

Choose a reason for hiding this comment

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

You can't, it is the other way around. Scan is an upgrade of Parsed.

Copy link
Contributor Author

@Happypig375 Happypig375 Jun 13, 2025

Choose a reason for hiding this comment

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

(|Parsed|_|) uses tryParse inside and (|Scan|_|) uses TryParseArray inside which uses tryParse. The logic is already shared.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

or you mean this active pattern should be named (|Parse|_|)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can I just call it (|Parse|_|)?

Copy link
Member

Choose a reason for hiding this comment

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

Good question. In general, active patterns should describe what something is, rather than what it does. That’s why we avoid naming them after actions like Parse.

We actually used to have an active pattern named Parse, but we deprecated it in favor of Parsed, to better reflect that it's identifying a parsed value, not performing the parsing itself.

So I’d recommend sticking with Parsed (or something similar that describes the result), to stay consistent with F# naming conventions and improve code readability.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reusing the name Parsed + F# naming convention for formatted strings f -> Parsedf

But it doesn't read well. Something like ParsedFormat would be too long especially when used across multiple lines. Maybe an adjective like Like per fsharp/fslang-suggestions#1426 would read better?

Copy link
Member

Choose a reason for hiding this comment

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

I propose, let's move the Active Pattern to its own issue/PR.
Then let's merge this PR, which is a different thing and it is already approved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Separated to #635

Copy link
Member

@gusty gusty left a comment

Choose a reason for hiding this comment

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

Nice job !

@gusty gusty merged commit a617ec4 into fsprojects:master Jun 14, 2025
4 checks passed
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.

2 participants