forked from DonJayamanne/pythonVSCode
-
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
Added a prompt asking users to enroll back in the insiders program #7484
Merged
Merged
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
9ebd8a0
Added functionality
f9ba06b
Added telemetry
ab7b5e9
Added tests
cdef35f
Some code reviews
2292854
Refactored functionality
11af2b0
Added doc
7b73c04
Refactored the tests
0d42a3e
Kode reviews
ce08248
Code reviews
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Added a prompt asking users to enroll back in the insiders program |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I see where you're coming from here, but I'm not sure these 3 new methods pull their own weight here. There's just not enough meat to them to pay for the extra cost to readers (e.g. extra boilerplate code, more indirection). Consider folding them back into
handleEdgeCases()
and perhaps even foldinghandleEdgeCases()
intohandleChannel()
. Here's one way to do that without having the clutter of the conditions: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.
To me each action changes based on the current state, I.e. state pattern. Gets rid of reach case statement and the complicated if condition.
E.g. tomorrow if we have another state we still modify both if conditions and the case statement, that's code smell.
Yes it might seem unnecessary, however I feel that code will be easier to follow, and each class does one thing. To me right now the following block alone is complicated,
if (installChannel === 'off' && !this.extensionChannelService.isChannelUsingDefaultConfiguration) { return 'promptToReEnroll'; }
, and to me it gets more complicated when you have more of those.And one way to solve that is
if this.shouldPromptToReEnroll
, then we end up with around 4 different methods that basically return a state.The break down further, we have a state of the class, next simply further using state pattern.
I like that because the state describes the next course of action, that's what we need. We have users who are in a particular state and we need to do something based on users state of extension.
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.
We could also go with a strategy pattern here, that would still simplify the code and accommodate new methods (instead of the switch statement). However looking at the two, state is there right fit (at least IMHO).
Basically both solve O. In SOLID.
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.
Was referring to @ericsnowcurrently 's review earlier :
So many if conditions in one place leads to harder testability and readability , which was the main reason for doing that. Having documentation for separate cases is also better when you use separate methods, rather than using if statements.
(Not to say I would've to refactor the entire test code as well, does not seem to be worth the effort for me)
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.
Agreed, we could go another step, self documenting code. Separate classes would make documentation unnecessary 😁