Skip to content
This repository was archived by the owner on Dec 5, 2024. It is now read-only.

Conversation

@StanleyGoldman
Copy link
Contributor

@StanleyGoldman StanleyGoldman commented Mar 24, 2018

DO NOT MERGE: This is part of #663

Depends on:

The validation methods ValidateKeychain and ValidateCurrentUserInternal load the requisite data and validate them. The methods that are calling these methods perform the same tasks to load the same data items and continue.

In order to reduce the re-querying the validation methods now load the data, validate the data and returns the data if valid or throw an exception if invalid as previously expected.

Refactoring method LoadKeychainInternal/ValidateKeychain to GetValidatedKeychainAdaper
Refactoring method ValidateCurrentUserInternal/GetCurrentUserInternal to GetValidatedGitHubUser

@StanleyGoldman StanleyGoldman requested a review from shana March 26, 2018 15:52
@shana shana changed the base branch from shana/fix-fix-fix to master March 26, 2018 18:51
StanleyGoldman and others added 5 commits March 26, 2018 18:53
Operational methods call the validation methods to validate data before they do their job.
The validation methods load the requisite data and validate them.
The operational methods then load the same data items and continue.

In order to reduce the re-querying the validation methods now load the data, validate the data and returns the data if valid.

Refactoring method `LoadKeychainInternal`/`ValidateKeychain` to `GetValidatedKeychainAdaper`
Refactoring method `GetCurrentUserInternal` to `GetValidatedGitHubUser`
@StanleyGoldman StanleyGoldman force-pushed the fixes/fix-keychain-missing-exception branch from 9f3c1b2 to 57fe7ef Compare March 26, 2018 22:55
@StanleyGoldman
Copy link
Contributor Author

I'm going to put a bit more into the explanation and resolution of this problem.

Credential might be null and we should be loading if there is no token
- LoadKeychainInternal is only called from ValidateKeychain
- ValidateKeychain should throw a TokenUsernameMismatchException if it
can see the user does not match
…n-missing-exception

# Conflicts:
#	src/GitHub.Api/Application/ApiClient.cs
#	src/GitHub.Api/Authentication/Keychain.cs
@StanleyGoldman StanleyGoldman changed the base branch from master to fixes/windows-creditals-missing March 27, 2018 14:36
@StanleyGoldman StanleyGoldman changed the base branch from fixes/windows-creditals-missing to master March 27, 2018 14:44
@StanleyGoldman StanleyGoldman mentioned this pull request Apr 2, 2018
16 tasks
Copy link
Member

@shana shana left a comment

Choose a reason for hiding this comment

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

I had a bunch of comments on this that I've translated into fixes in #667

@shana shana merged commit 60da5ec into master Apr 5, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants