★ wanayoo — archive 1999 https://github.com/PowerShell/PowerShellEditorServices/pull/947Nouvelle recherche | Portail wanayoo
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

Fix silent failure by using Id for dictionary since multiple pages could have the same title #947

Merged
merged 2 commits into from Apr 26, 2019

Conversation

@TylerLeonhardt
Copy link
Member

@TylerLeonhardt TylerLeonhardt commented Apr 25, 2019

I want to be clear that this is only fixing a bug where:

$v = New-VSCodeHtmlContentView -Title foo
$v = New-VSCodeHtmlContentView -Title foo

the first time would be an object we expect, the second time, $v would be null because .Add throws if the key already exists. After switching it to use Id, there should be no collision.

Ideally this feature could use a Get-VSCodeHtmlContentView that pulls from that Dictionary... but that's out of scope.

Copy link
Collaborator

@SeeminglyScience SeeminglyScience left a comment

LGTM!

@bergmeister
Copy link
Collaborator

@bergmeister bergmeister commented Apr 25, 2019

LGTM but the PR title could say something about the bug that it fixes on a higher level, which is the purpose of the PR

Copy link
Collaborator

@rkeithhill rkeithhill left a comment

LGTM

@TylerLeonhardt TylerLeonhardt changed the title Use Id for dictionary since multiple pages could have the same title Fix silent failure by using Id for dictionary since multiple pages could have the same title Apr 26, 2019
@PowerShell PowerShell deleted a comment Apr 26, 2019
@PowerShell PowerShell deleted a comment Apr 26, 2019
@TylerLeonhardt TylerLeonhardt merged commit 358dd07 into PowerShell:master Apr 26, 2019
8 checks passed
8 checks passed
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
PowerShellEditorServices-CI Build #PR-947-20190426.01 succeeded
Details
PowerShellEditorServices-CI (PowerShell 5.1 | Windows 10) PowerShell 5.1 | Windows 10 succeeded
Details
PowerShellEditorServices-CI (PowerShell 6 | Ubuntu) PowerShell 6 | Ubuntu succeeded
Details
PowerShellEditorServices-CI (PowerShell 6 | Windows 10) PowerShell 6 | Windows 10 succeeded
Details
PowerShellEditorServices-CI (PowerShell 6 | macOS) PowerShell 6 | macOS succeeded
Details
PowerShellEditorServices-secret-scan #PR-947-20190426.01 succeeded
Details
license/cla All CLA requirements met.
@TylerLeonhardt TylerLeonhardt deleted the TylerLeonhardt:use-id-for-dict branch Apr 26, 2019
TylerLeonhardt added a commit that referenced this pull request Apr 26, 2019
…uld have the same title (#947)

* Use Id for dictionary since multiple pages could have the same title

* codacy got me
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.