Changes to OAuth service and update undefined state by karlclement · Pull Request #585 · SquireAI/codex-vscode

karlclement · 2024-03-12T16:46:44Z

This pull request introduces a series of code quality improvements across multiple services. In oauth.service.ts, variable names have been refactored for clarity, code formatting enhanced, and a TODO added to address future simplification of timeout rejection logic. The undefinedTextEditor.service.ts has been updated for better readability with new lines, a streamlined if-statement, and a more flexible method parameter type, while user.service.ts now includes a reminder to clean up comments.

File Changes

oauth.service.ts: Refactored variable names and improved code formatting. Added a TODO comment regarding simplification of a timeout rejection logic.
undefinedTextEditor.service.ts: Inserted new lines for readability, simplified an if-statement, and changed a parameter type to 'any' in a method signature.
user.service.ts: Added a TODO comment to remind of cleaning up comments in the future.

karlclement · 2024-03-12T16:48:06Z

 import { PromiseAdapter, promiseFromEvent } from "../helpers/eventEmitter.helper";
-import { ErrorResponse, IOAuthService, OAuthPath, OAuthPlatform, UserInfo } from "../models/shared/IOAuthService";
+
+import {ErrorResponse, IOAuthService, OAuthPath, OAuthPlatform, UserInfo} from "../models/shared/IOAuthService";


Break this import into multiple lines, way too long

@echolayer-stag can you break this import into multiple lines

Sorry, I couldn't find a solution for this task. I'll polish up on the docs for next time.

karlclement · 2024-03-12T16:48:33Z

  public async authenticate(oAuthPath: OAuthPath): Promise<UserInfo | ErrorResponse> {
    const oAuthPlatform: OAuthPlatform = oAuthPath === OAuthPath.google ? OAuthPlatform.google : OAuthPlatform.github;
-    const eventEmitter = OAuthUrlHandler.getInstance().getEventEmitter();
+    const ee = OAuthUrlHandler.getInstance().getEventEmitter();


Change for a much more descriptive variable name

@echolayer-stag rename this to be a much more descriptive variable name

EchoLayer posted a change for your review: #586

karlclement · 2024-03-12T16:49:18Z

    // before completing it.
    if (!OAuthService.codeExchangePromise) {
-      OAuthService.codeExchangePromise = promiseFromEvent(eventEmitter.event, this.postAuthorization());
+      OAuthService.codeExchangePromise = promiseFromEvent(


Not sure why this is multiple lines

@echolayer-stag can you collapse this all into one line

EchoLayer posted a change for your review: #588

karlclement · 2024-03-12T16:49:36Z

      OAuthService.codeExchangePromise.promise,
      new Promise<ErrorResponse>((_, reject) =>
        setTimeout(() => {
+          // TODO we should make this alot simpler


We fixed this already, maybe add some comments

@echolayer-stag remove this TODO and instead add comments to the return Promise.race on how it works

EchoLayer posted a change for your review: #589

karlclement · 2024-03-12T16:50:03Z

      this.clearCodeExchangePromise();
-      const parsedQuery = this.parseQuery(uri) as AuthorizeResponse | ErrorResponse;
-      if ((parsedQuery as AuthorizeResponse).code) {
+      const parsed_query = this.parseQuery(uri) as AuthorizeResponse | ErrorResponse;


We don't use snake case in this file

@echolayer-stag change this to camelCasing please

EchoLayer posted a change for your review: #587

karlclement · 2024-03-12T16:50:32Z


  private parseQuery(uri: vscode.Uri) {
    return uri.query.split("&").reduce((prev: any, current) => {
+      // important to split


Not a helpful comment to explain

@echolayer-stag can you remove this unhelpful comment and instead actually comment what this code block is doing

EchoLayer posted a change for your review: #590

karlclement · 2024-03-12T16:51:05Z

-    if (force) {
-      await this.getContexts(force);
-    }
+    if (force) await this.getContexts(force);


This shouldn't all be on one line

@echolayer-stag split this into multiple lines so it's easier to read for my future self

EchoLayer posted a change for your review: #595

karlclement · 2024-03-12T16:51:31Z

  }

-  public async getDefaultCodexServiceForRepo(repositoryService: RepositoryService): Promise<CodexService> {
+  public async getDefaultCodexServiceForRepo(repositoryService: any): Promise<CodexService> {


Add some comments explaining what this is for

@echolayer-stag add comments explaining what this is for

EchoLayer posted a change for your review: #591

karlclement · 2024-03-12T16:52:06Z

    return this.ccl.getContextsService();
  }

+  // TODO clean up comments


Would replace this with a valid comment, in fact this file doesn't have any comments explaining the functions

@echolayer-stag remove this todo and add comments that explain all functions in this file

EchoLayer posted a change for your review: #593

baell · 2024-03-13T18:38:01Z

+      OAuthService.codeExchangePromise = promiseFromEvent(
+        ee.event, 
+        this.postAuthorization()
+      );


@echolayer-stag can you collapse this all into one line

EchoLayer posted a change for your review: #594

saumilptl · 2024-03-19T20:42:22Z

@@ -16,21 +17,25 @@ export class OAuthService implements IOAuthService {
   */
  public async authenticate(oAuthPath: OAuthPath): Promise<UserInfo | ErrorResponse> {


@echolayer rename this function to oAuthenticate

EchoLayer posted a change for your review: #596

EchoLayer posted a change for your review: #597

EchoLayer posted a change for your review: #598

Karl Clement added 2 commits March 12, 2024 09:42

Changes for demo

dcb95a4

Added change to user.service

be573a7

karlclement commented Mar 12, 2024

View reviewed changes

karlclement added the do not merge Do not merge this branch label Mar 12, 2024

baell reviewed Mar 13, 2024

View reviewed changes

This was referenced Mar 13, 2024

[EchoLayer (Stag)] Changes to OAuth service and update undefined state #594

Closed

[EchoLayer (Stag)] Changes to OAuth service and update undefined state #595

Closed

saumilptl reviewed Mar 19, 2024

View reviewed changes

SquireAI deleted a comment Mar 19, 2024

This was referenced Mar 19, 2024

[EchoLayer] Changes to OAuth service and update undefined state #596

Open

[EchoLayer] Changes to OAuth service and update undefined state #597

Open

[EchoLayer] Changes to OAuth service and update undefined state #598

Open

SquireAI deleted a comment from squireai-staging Bot Mar 28, 2024

		@@ -16,21 +17,25 @@ export class OAuthService implements IOAuthService {
		*/
		public async authenticate(oAuthPath: OAuthPath): Promise<UserInfo \| ErrorResponse> {

Conversation

karlclement commented Mar 12, 2024 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

File Changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

squireai-staging Bot Mar 13, 2024

Choose a reason for hiding this comment

Uh oh!

karlclement Mar 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

squireai-staging Bot Mar 12, 2024

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

squireai-staging Bot Mar 13, 2024

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

squireai-staging Bot Mar 13, 2024

Choose a reason for hiding this comment

Uh oh!

karlclement Mar 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

squireai-staging Bot Mar 12, 2024

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

squireai-staging Bot Mar 13, 2024

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

squireai-staging Bot Mar 13, 2024

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

squireai-staging Bot Mar 13, 2024

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

squireai-staging Bot Mar 13, 2024

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

squireai-staging Bot Mar 13, 2024

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

karlclement commented Mar 12, 2024 •

edited by ghost

Loading

karlclement Mar 12, 2024 •

edited

Loading

karlclement Mar 12, 2024 •

edited

Loading