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

Hidden members should not be shown in QuickInfo #73

Closed
wants to merge 5 commits into from

Conversation

forki
Copy link
Contributor

@forki forki commented Jan 21, 2015

this fixes #50

I added a testcase and I used CheckFSharpAttributesForUnseen to ask for the hidden attribute.

I had to split CheckFSharpAttributesForUnseen since NicePrint.fs has a mode denv.showObsoleteMembers which allows to show obsolete members but CheckFSharpAttributesForUnseen checked for hidden and obsolote.

@dsyme
Copy link
Contributor

dsyme commented Jan 21, 2015

Great! Looks good to me, though see one comment below. Especially good to see the Language Service unit test added.

@@ -1505,6 +1505,7 @@ module private TastDefinitionPrinting =
not (isInterfaceTy denv.g oty)
| [] -> true)
|> List.filter (fun v -> denv.showObsoleteMembers || not (HasFSharpAttribute denv.g denv.g.attrib_SystemObsolete v.Attribs))
|> List.filter (fun v -> not (Infos.AttributeChecking.CheckFSharpAttributesForHidden denv.g v.Attribs))
Copy link
Contributor

Choose a reason for hiding this comment

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

The NicePrint.fs code is used to show the signature of things when emitting signature files using the "-sig:file" option. So I think you need to show obsolete, hidden and experimental members in that case, because the signature file should be "logically complete".

For this, you should add a flag "showHiddenMembers" to DisplayEnv with default value "false", and set this to "true" in BuildInitialDisplayEnvForDocGeneration.

I actually think "showObsoleteMembers" should match this and be "false" by default, but set to "true" in BuildInitialDisplayEnvForDocGeneration. I don't think we want obsolete members shown in class/module tooltips.

Finally, rename BuildInitialDisplayEnvForDocGeneration to BuildInitialDisplayEnvForSigFileGeneration.

@dsyme
Copy link
Contributor

dsyme commented Jan 21, 2015

@latkin @KevinRansom - I went looking for an "under review" label to label the pull request with - I kind of like it if all issues and pull requetss get a label of some kind - if you agree could you create a label for this? thanks!

@forki
Copy link
Contributor Author

forki commented Jan 21, 2015

yep seems you are correct. obsolete methods are still shown in QuickTips but not in IntelliSense.

image

image

I'll append a fix.

@forki
Copy link
Contributor Author

forki commented Jan 21, 2015

I added a test for the obsolete case and added the changes you suggested.

But now we have a new issue: the test is still red.
On my local repo I added a bit of printf debugging and saw that the Obsolete-attribute is not found in https://github.com/Microsoft/visualfsharp/pull/73/files#diff-4a6abf380cb46c97f62d8f796adeefdfR1507 since v.Attribs is empty.

Any ideas?

@dsyme
Copy link
Contributor

dsyme commented Jan 21, 2015

I took a look and didn't spot any problem with the code. Do both tests fail or just the "obsolete" one? Is v.Attribs empty for both Print1 and Print2?

@forki
Copy link
Contributor Author

forki commented Jan 21, 2015

only the obsolete test fails. Both properties (Print1 and Print2) have 0 attribs.

If I add the HiddenAttribute to Print1 in the obsolete test, I get 1 attribute (Obsolete attr is still missing) and Print1 is no longer displayed.

So somehow it doesn't find the ObsoleteAttribute, but the HiddenAttribute works.

@dsyme
Copy link
Contributor

dsyme commented Jan 21, 2015

You may have to open "System" or use "System.Obsolete"

@forki
Copy link
Contributor Author

forki commented Jan 21, 2015

scratch that. I cleaned my repo, added System. and rebuilt everything from scratch.
All green. :shipit: ;-)

@dsyme
Copy link
Contributor

dsyme commented Jan 21, 2015

OK, great, then this looks good to go.

@forki
Copy link
Contributor Author

forki commented Jan 21, 2015

just for my understanding:

the attribute wasn't found since the code didn't compile.
The QuickInfo still kind-of worked on the uncompilable code. It just processed everything without the attribute.

Is that correct?

@latkin latkin closed this in ac55622 Jan 22, 2015
@forki forki deleted the tooltip branch January 22, 2015 09:54
@forki
Copy link
Contributor Author

forki commented Jan 22, 2015

Yes! First accepted bug fix.

muscle

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.

None yet

3 participants