Skip to content

Send event on referencing large file#26197

Merged
sheetalkamat merged 1 commit into
masterfrom
largeFileEvent
Aug 13, 2018
Merged

Send event on referencing large file#26197
sheetalkamat merged 1 commit into
masterfrom
largeFileEvent

Conversation

@sheetalkamat
Copy link
Copy Markdown
Member

Event when referencing large file as per #26169 (comment)

@RyanCavanaugh RyanCavanaugh requested review from amcasey and removed request for amcasey August 3, 2018 22:51
@amcasey
Copy link
Copy Markdown
Member

amcasey commented Aug 3, 2018

If this were turning into telemetry (which I don't believe it is), the file size would be okay but the file path would not.

@amcasey
Copy link
Copy Markdown
Member

amcasey commented Aug 3, 2018

@armanio123, should VS respond to this event? Do we want to show an info message or something? This seems similar to LS disabled.

@armanio123
Copy link
Copy Markdown
Contributor

I think it might be a good idea to show an info message, just to make the user aware of what's happening on his project. Nevertheless, I don't think the feedback we have received relates to this scenario specifically or if we can get an action from the user by showing the message.

@sheetalkamat
Copy link
Copy Markdown
Member Author

Is this good to merge ?

@amcasey
Copy link
Copy Markdown
Member

amcasey commented Aug 10, 2018

I have no objections, but Ryan only added me by accident. I know @sandersn is adding an event, so he might have thoughts.
As I mentioned above, as long as this doesn't turn into telemetry, I have no GDPR objections.

@sheetalkamat
Copy link
Copy Markdown
Member Author

@RyanCavanaugh @sandersn Ok to merge ?

@sandersn
Copy link
Copy Markdown
Member

I’m interested in this PR as an example of how to add a new flag, but I’ll take a look at the other parts now.

Copy link
Copy Markdown
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

Looks good as far as I understand the code; just one question about code duplication.

Comment thread src/server/protocol.ts
openFiles: string[];
}

export type LargeFileReferencedEventName = "largeFileReferenced";
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why is this code duplicated? I guess that’s one public and the other private, but why aren’t they shared?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

That's because we want to be able to build protocol.ts separately as a standalone to protocol.d.ts which editors consume.

@sheetalkamat sheetalkamat merged commit f2011ce into master Aug 13, 2018
@sheetalkamat sheetalkamat deleted the largeFileEvent branch August 13, 2018 18:14
@sandersn sandersn mentioned this pull request Aug 14, 2018
@microsoft microsoft locked as resolved and limited conversation to collaborators Oct 21, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants