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

Don't print the declaring type for RQA union cases #10083

Merged
merged 2 commits into from
Sep 14, 2020
Merged

Conversation

cartermp
Copy link
Contributor

@cartermp cartermp commented Sep 6, 2020

fixes #9357

Unfortunately, both %A and %s do the same thing for union cases, so if anyone is relying on stringifying a union case for anything they can't switch to a "better" way unless they override ToString().

Ideally people would not rely on this, since the prupose of this functionality is to provide structured plaintext output in tools. But the reality is that it is being used for more than that.

@abelbraaksma
Copy link
Contributor

Now that this is considered a regression, I propose to merge this into the VS 16.7.x branch as well and not just into future branches, since otherwise we may keep seeing reports on this cropping up until long term support is over.

@cartermp
Copy link
Contributor Author

cartermp commented Sep 6, 2020

We'll see if there are reports. We generally avoid servicing unless it is absolutely necessary, and VS telemetry shows that users overwhelmingly update instead of stay on older versions, even if it's technically marked LTS.

@cartermp cartermp requested a review from dsyme September 6, 2020 15:05
@dsyme
Copy link
Contributor

dsyme commented Sep 7, 2020

I think we should be more nuanced here

  1. I can see the case for reverting for generated ToString() - that should not have changed

  2. I'm less keen on reverting for %A. We make no guarantees about %A except that the output is human readable, and the new output is definitely more human readable. .

  3. I'm against reverting for FSI stdout output printing (which the current PR is doing). That absolutely has to prioritise human readability and also the ability to round-trip some data, so the qualifications should stay

Technically separating 1/2 from 3 is simple. Separating 1 from 2/3 is a little harder (it needs another flag passed through to printf in the format string, the only way to plumb such flags through).

@cartermp
Copy link
Contributor Author

cartermp commented Sep 7, 2020

I'd be fine with that I suppose. Note that the same nuance should probably apply for enums, especially when you consider that they need to be fully qualified anyways. Though that gets even more tricky if you consider open type MyEnum.

@abelbraaksma
Copy link
Contributor

abelbraaksma commented Sep 7, 2020

Though that gets even more tricky if you consider open type MyEnum.

Note that enum values, other than DU cases, have a clear string representation: the case name only. In FSI, they are currently represented in the default output a little bit different than other types, but in regular programming with sprintf "%A", sprintf "%O" and string MyEnum.X they always format as the field-name only, without qualification. I don't think we should change that. Likewise, any stringification should ideally be transparent w.r.t. any opens or open types.

> type MyEnum = One = 1 | Two = 2;;
type MyEnum =
  | One = 1
  | Two = 2

> MyEnum.One;;
val it : MyEnum = One {value__ = 1;}

> string MyEnum.One;;
val it : string = "One"

> sprintf "%A" MyEnum.One;;
val it : string = "One"

And with open type:

> open type MyEnum;;
> One;;
val it : MyEnum = One {value__ = 1;}

> MyEnum.One;;
val it : MyEnum = One {value__ = 1;}

@cartermp
Copy link
Contributor Author

@dsyme I think we can tweak that later. But right now I just want to get this in so that we don't ship the regression by release.

@dsyme
Copy link
Contributor

dsyme commented Sep 14, 2020

@dsyme I think we can tweak that later. But right now I just want to get this in so that we don't ship the regression by release.

ok

@dsyme dsyme merged commit 8b8b2d1 into main Sep 14, 2020
@brettfo brettfo deleted the revert-union-case-stuff branch October 29, 2020 22:29
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.

Breaking change for DU with [<RequireQualifiedAccess>] formatting in 4.7.2
3 participants