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

Improved nice print to give literal values #10398

Merged
merged 2 commits into from
Nov 9, 2020

Conversation

En3Tho
Copy link
Contributor

@En3Tho En3Tho commented Nov 6, 2020

This change enables pretty printing Const values. Need some help with deciding printing manner (currently I'm using parentheses to show literal value). Also, I'm not sure I used WordL api right but I hope I did.
Screenshots are from JetBrains Rider.
Seems like mapping from NumbericLiteral could be fixed in Rider as it reports wrong color for mkNumericLiteral, but okay with mkStringLiteral:
image
image

@En3Tho En3Tho changed the title Improved nice print to give literal values [WIP] Improved nice print to give literal values Nov 6, 2020
@auduchinok
Copy link
Member

auduchinok commented Nov 6, 2020

Seems like mapping from NumbericLiteral could be fixed in Rider as it reports wrong color for mkNumericLiteral

Good timing, it has been fixed yesterday. 🙂

@En3Tho
Copy link
Contributor Author

En3Tho commented Nov 6, 2020

Yep, all good now! Thanks @auduchinok
image

src/fsharp/NicePrint.fs Outdated Show resolved Hide resolved
src/fsharp/NicePrint.fs Outdated Show resolved Hide resolved
@saul
Copy link
Contributor

saul commented Nov 6, 2020

The bracket syntax is a bit strange IMO. Would it be better for the tooltip to be:

val SomeFloat : float32 = 15.23f

@En3Tho
Copy link
Contributor Author

En3Tho commented Nov 6, 2020

@auduchinok pointed out that val name could be reported as literal and highlighted accordingly. I can try address that in another PR.
image
Also, there are lots of static values like WordL.equals, WordL.colon, but they are not used in NicePrint.layoutNonMemberVal function. Instead they are being created anew every time. Should I try to find such places and replace keywords and language elements with statics? In another PR.
image
All that if this gets merged of course :0
What about tests on this thing? Do I need to add something?

@En3Tho
Copy link
Contributor Author

En3Tho commented Nov 6, 2020

@saul

The bracket syntax is a bit strange IMO. Would it be better for the tooltip to be:

Already fixed :)

@En3Tho En3Tho requested a review from cartermp November 6, 2020 19:38
@cartermp
Copy link
Contributor

cartermp commented Nov 6, 2020

val name could be reported as literal and highlighted accordingly

Yeah, this is where things get weird. Is it more correct to show it as a normal val in tooltips, since it's a value? Or go much further on differentiating it as a literal? I would personally prefer the latter but I know that it's quite intentional that there's much less distinction between things in the F# design, and @dsyme has expressed as much in other PRs about tooltips like this one: #9563

@auduchinok
Copy link
Member

We could probably classify it properly at the FCS level so it would be up to the tooling if the additional info should be used. VS integration could then ignore the additional colors and fallback to the current ones where needed.

@cartermp
Copy link
Contributor

cartermp commented Nov 6, 2020

Oh, I would personally color it differently in VS. That was the crux of my argument in favor of differentiating more, that editors have more options available to them.

@cartermp
Copy link
Contributor

cartermp commented Nov 7, 2020

What about tests on this thing? Do I need to add something?

Testing is definitely lacking on this front. I suppose we'd be happy to have the start of a test suite here, but I don't think that's a prerequisite to get this merged.

@En3Tho En3Tho changed the title [WIP] Improved nice print to give literal values Improved nice print to give literal values Nov 7, 2020
@En3Tho
Copy link
Contributor Author

En3Tho commented Nov 9, 2020

@cartermp Hey Phillip! Can you merge this please or there are some more things I need to do before that?

@vzarytovskii vzarytovskii merged commit eda62fe into dotnet:main Nov 9, 2020
@En3Tho En3Tho deleted the features_literals_in_tooltips branch November 12, 2020 09:03
@cartermp cartermp added this to the 16.9 milestone Nov 17, 2020
nosami pushed a commit to xamarin/visualfsharp that referenced this pull request Feb 23, 2021
* Improved nice print to give literal values

* Addressed CR
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.

5 participants