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

Remove duplicated navigation bar items for properties with getters and setters #2146

Merged
merged 4 commits into from
Jan 3, 2017

Conversation

vasily-kirichenko
Copy link
Contributor

This fixes #1 and #2 from #2132 and adds accessibility aware glyphs:

image

| [SynMemberDefn.Member(memberDefn=Binding(headPat=SynPat.LongIdent(lid1, Some(info1),_,_,_,_)) as binding1)
SynMemberDefn.Member(memberDefn=Binding(headPat=SynPat.LongIdent(lid2, Some(info2),_,_,_,_)) as binding2)] ->
// ensure same long id
assert((lid1.Lid,lid2.Lid) ||> List.forall2 (fun x y -> x.idText = y.idText))
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the assert doing here? Are we just throwing if this is not fulfilled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've ported this code from here https://github.com/Microsoft/visualfsharp/blob/master/src/fsharp/vs/ServiceParseTreeWalk.fs#L418-L454 I think that if this assertion violated, it means that something very wrong happened with the parser and we must see it in debug configuration.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok

Copy link
Contributor

@cartermp cartermp left a comment

Choose a reason for hiding this comment

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

Nice approach.

@@ -26,10 +26,10 @@ type FSharpNavigationDeclarationItemKind =

/// Represents an item to be displayed in the navigation bar
[<Sealed>]
type FSharpNavigationDeclarationItem(uniqueName: string, name: string, kind: FSharpNavigationDeclarationItemKind, glyph: GlyphMajor, range: range, bodyRange: range, singleTopLevel:bool) =
type FSharpNavigationDeclarationItem(uniqueName: string, name: string, kind: FSharpNavigationDeclarationItemKind, glyph: GlyphMajor, range: range,
Copy link
Member

Choose a reason for hiding this comment

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

@vasily-kirichenko ---
I suppose we should start formatting lines so that they can be seen in a GitHub PR.

I know I am the worst offender for long lines, but, perhaps we should limit line lengths to 120 characters max. Although this is a terrible waste on my new monitor which is currently showing a 463 characters wide editor display :-(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm for 120 or maybe 140 characters lines 👍

Maybe we should add this code style rule to devguide.md

Copy link
Contributor

Choose a reason for hiding this comment

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

I definitely support that change.

@KevinRansom KevinRansom merged commit a24a560 into dotnet:master Jan 3, 2017
nosami pushed a commit to xamarin/visualfsharp that referenced this pull request Jan 26, 2022
…d setters (dotnet#2146)

* remove duplicated navigation bar items for properties with getters and setters

* fix glyphs for class let bindings

* better navigation bar glyphs

* fix accessibility for members and let bindings
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