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

Using FQN instead of adding import during ENC session #35194

Merged
merged 1 commit into from
Apr 25, 2019

Conversation

genlu
Copy link
Member

@genlu genlu commented Apr 23, 2019

@tmat This feels like a better approach than listening to the debugger events, as completion provider is mostly stateless. What do you think?

FYI @dpoeschl This is not intended to be merged before snap today, just need to target some branch. Will decide and retarget if necessary to the proper branch later.

@genlu genlu added the Area-IDE label Apr 23, 2019
@genlu genlu requested a review from a team as a code owner April 23, 2019 00:47
@jasonmalinowski
Copy link
Member

The code doesn't look suspicious, but out of curiosity how much work is it to just allow imports to be added during an E&C session? I can imagine the difficulty comes from potentially rebinding other identifiers in the file, but it seems like we should be able to handle that or know if it's going to be a problem...

@tmat
Copy link
Member

tmat commented Apr 23, 2019

@jasonmalinowski It is indeed not impossible to implement and I would like to see us supporting it since it's the number one rude edit that customers run into.

I can imagine the difficulty comes from potentially rebinding other identifiers

Correct. We'd need to find out what methods are affected and recompile them, or report rude edits if we can't recompile a method that's affected.

@jasonmalinowski
Copy link
Member

jasonmalinowski commented Apr 23, 2019

Is there a tracking item for this work we can point to this code (or stick a comment here) in hopes that if we fix it we remove this workaround?

@tmat
Copy link
Member

tmat commented Apr 23, 2019

#10043

@genlu genlu force-pushed the ImportCompletionENC branch from 75e47d8 to 612ae1e Compare April 23, 2019 17:38
@genlu
Copy link
Member Author

genlu commented Apr 23, 2019

FYI @jinujoseph @vatsalyaagrawal This needs QB approval

@jinujoseph jinujoseph added this to the 16.1.P3 milestone Apr 23, 2019
@genlu genlu changed the base branch from master to dev16.1 April 23, 2019 18:16
@genlu genlu mentioned this pull request Apr 23, 2019
@jinujoseph jinujoseph modified the milestones: 16.1.P3, 16.1 Apr 24, 2019
@genlu
Copy link
Member Author

genlu commented Apr 25, 2019

@jinujoseph It seems we have some issue with the integration test machines. I decided to merge this because:

  1. 2 out of 4 legs of integration tests passed, before we start hitting this issue
  2. This change can't affect integration tests, since import completion is not executed there.

@genlu genlu merged commit 36d71ef into dotnet:dev16.1 Apr 25, 2019
@genlu genlu deleted the ImportCompletionENC branch April 25, 2019 19:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants