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

Implement interface code fix #2112

Merged
merged 7 commits into from
Dec 27, 2016
Merged

Conversation

dungpa
Copy link
Contributor

@dungpa dungpa commented Dec 26, 2016

Move the feature from VFPT:

image

@vasily-kirichenko
Copy link
Contributor

Fantastic :)

@vasily-kirichenko
Copy link
Contributor

It does not add the closing curly brace:

1

@vasily-kirichenko
Copy link
Contributor

BTW, the preview is particular useful for this feature :)

@smoothdeveloper
Copy link
Contributor

Regarding performance, will there be a way to disable the features that the user deem not useful like in VFPT?

I'm concerned that having all the features in VS but no way to disable those might lead to bigger performance concerns, I know some F# users don't install VFPT because of this.

@vasily-kirichenko
Copy link
Contributor

@smoothdeveloper that were not the features that makes VS slower with VFPT enabled. It was FCS itself. Now we have FCS in VS built-in and its older duplicate has gone. Enabling / disabling features like the one implemented in this PR does not affect performance.

@smoothdeveloper
Copy link
Contributor

@vasily-kirichenko thanks, figured that separate FCS was hurting a lot as well, looking forward to try that new version soon :)

@cartermp
Copy link
Contributor

In general you'll have slow performance on very large files (a few thousand lines), which is something we're definitely going to address after VS RTM release. Can't afford the time to do it now, but it's a priority. I've noticed that if files stay under 1k lines, it's pretty fast and definitely usable.

@vasily-kirichenko
Copy link
Contributor

@smoothdeveloper having that said, we are definitely planning to add a setting dialog, like we had in VFPT, see #2111 (comment)

@saul
Copy link
Contributor

saul commented Dec 26, 2016

This looks great @dungpa :)

Only thing to note is that the "Implement Interface Explicitly" should be in normal sentence case for consistency with the other code fixes (i.e. "Implement interface explicitly"). Also maybe the verbose option should be more clear - I can't tell without looking at the preview what it does. I also think that that "Stub missing interface methods" reads better.

@dungpa
Copy link
Contributor Author

dungpa commented Dec 26, 2016

@saul Thanks. I renamed the labels to "Implement interface explicitly" (meaning having type annotations on arguments and return types) and "Implement interface explicitly without type annotation".

@vasily-kirichenko That part has never been done in VFPT. I will have a look at implementing it.

@KevinRansom
Copy link
Member

KevinRansom commented Dec 26, 2016

This is great.

I noticed that: if I format the first line like this, I get no lollipop fixer?
screen shot 2016-12-26 at 3 36 38 pm
Whereas when I format it on a separate line the fixer appears:
screen shot 2016-12-26 at 3 33 32 pm

Is that intentional?

Kevin

@dsyme
Copy link
Contributor

dsyme commented Dec 27, 2016

Perhaps FSharp.Core should contain

let nyi () = raise (NotImplementedException())

However we could only use that in codegen if the appropriate version of FSharp.Core was referenced

@KevinRansom
Copy link
Member

@dsyme I don't think so ... the purpose of the nyi is a reminder to the developer to add code here.

Copy link
Member

@KevinRansom KevinRansom left a comment

Choose a reason for hiding this comment

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

LGTM

@KevinRansom KevinRansom merged commit d11d86c into dotnet:master Dec 27, 2016
@dungpa dungpa deleted the implement-interface branch December 28, 2016 00:14
nosami pushed a commit to xamarin/visualfsharp that referenced this pull request Jan 26, 2022
* Create InterfaceStubGenerator service

* Add a prototype of ImplementInterface code fix

* Fix indentation calculation

* Support both interface declarations and object expressions

* Handle already implemented members

* Localize lightbulb text

* Rename labels
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.

8 participants