-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Display a message with options when default Mac interpreter is selected #2570
Display a message with options when default Mac interpreter is selected #2570
Conversation
@d3r3kk |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I really do like how you've devised the diagnostics routines for the extension. File naming is one of the things I've not yet fully come to terms yet with in this codebase - most of the codebases I've worked on previously have a 1:1 relationship between class defined within.
@@ -0,0 +1,116 @@ | |||
// Copyright (c) Microsoft Corporation. All rights reserved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One question here: Why don't we name the files with the name of the class defined within it? Makes it a bit more discoverable to find things using Ctrl+p
instead of Ctrl+Shift+f
then searching...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Not a blocker, just asking why)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No reason, just that in JS/TS I haven't come across this rule.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's totally fair, and what I expected. I am 100% certain that the rules in my prior codebases were enforced socially, perhaps canonically-ish in C#/C++.
Yes, please do open a different PR for the change you describe here. Would like to chat about it with you first to get an understanding of what you mean. (I think I follow, but want to be sure). |
Fixes #1689
package.json
are pinned (e.g."1.2.3"
, not"^1.2.3"
for the specified version)package-lock.json
has been regenerated by runningnpm install
(if dependencies have changed)