- Notifications
You must be signed in to change notification settings - Fork825
Fixing a few edge cases around display name (+cleanup)#13552
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
@dsyme What do you think about normalizing names for I guess it should be discussed in the style guide repo instead and I'd be happy to move there, but it would be great to have these names follow the common conventions anyway. |
I can't really see us doing this. I understand the situation but across the major Checking and CodeGen and Optimization files I've found code clearer if the major recursive and/or revealed-in-signature routines (Gen*, Optimize*, Tc*) are capitalized, while utility functions from TypedTreeOps etc. are generally camelCase (though if functionality forms a logical unit, rather than utilities, they are sometimes PascalCase) There's no clear dividing line for this and for PrettyNaming we're sort of caught in the middle - a somewhat messy set of lowerCase names in the implementation and the PascalCase ones in the implementation. In some ways the PascalCase indicates an intent to clean things up into a coherent whole. |
@@ -35,6 +35,9 @@ These release notes track our current efforts to document changes to the F# proj | |||
* `SynMeasure` was extended with [SynMeasure.Paren](https://fsharp.github.io/fsharp-compiler-docs/reference/fsharp-compiler-syntax-synmeasure.html#Paren) case. | |||
* Dynamic expressions (like `x?y`) are now represented as [SynExpr.Dynamic](https://fsharp.github.io/fsharp-compiler-docs/reference/fsharp-compiler-syntax-synexpr.html#Dynamic) in the Untyped Syntax Tree. | |||
* Members with `get` and/or `set` are now represented as [SynMemberDefn.GetSetMember](https://fsharp.github.io/fsharp-compiler-docs/reference/fsharp-compiler-syntax-synmemberdefn.html#GetSetMember) in the Untyped Syntax Tree. | |||
* `DoesIdentifierNeedBackticks` is removed, it should always be sufficient to call `NormalizeIdentifierBackticks` or else call something in `PrettyNaming` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@auduchinok@baronfel Can you check this? We think it is OK but would like to know if Rider or VSCode are depending on these
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
So there will be followups on that PR, if we find issues we can revert some of these changes in those followups
auduchinokAug 18, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@dsyme It seems it won't affect us. I'll raise an issue if anything doesn't work as expected.
Thanks for double-checking it for other tools! 🙂
@psfinaki So this is looking good, I'm glad to see it green I'm surprised that no other tests failed. There are quite a lot of places where I believe we will have fixed bugs, e.g. quick info for active pattern names that contain spaces, Should we do a pass together where we review the code and characterise these in test cases? I'll go through the code now and mark up bugs I believe fixed thanks |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
dsyme commentedJul 29, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I notice that we didn't end up fixing the problems with module``Thingwith space``=letx=1 where double-ticks are not added to the full path reported. Similarly hovering over typeA=classendmoduleA=letx=1 Could you list this as a separate bug? I'll write some notes about how to fix it:
thanks |
If you could add test cases for the above that would be great. Once that's done this is ready to go :) |
|
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
fixes#13061
fixes#13593
fixes#13609
fixes#13610
The cleanup is related to logical names and display names, there are also some docs added.