Skip to content

DO NOT MERGE #599

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.

Sign up for GitHub

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

Open
karlclement wants to merge 2 commits into
base: derrick/cod-1379-if-there-is-no-remote-on-a-local
Choose a base branch
from
Open

DO NOT MERGE #599

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cab9ee9
Always check for origin before fallback to first remote
derrickpersson Jul 19, 2022
1301000
Removing else
derrickpersson Jul 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion codex-vscode/environments/.env.example
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,4 +17,5 @@ LOG_MAX_FILES=10
LOG_LEVEL=error

SENTRY_DNS=""
SENTRY_AUTH_TOKEN=""
SENTRY_AUTH_TOKEN=""
SENTRY_LOG_LEVEL="error"
8 changes: 7 additions & 1 deletion codex-vscode/src/helpers/logging.helper.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,12 @@ const consoleFormat = winston.format.printf(({ level, message, timestamp, ...met
if (meta) {
if (meta.error as Error) {
message += `\n${level}: ${meta.error?.message}\n${meta.error?.stack}`;
} else {
try {
message += `\n${JSON.stringify(meta, undefined, 2)}`;
} catch (error) {
// Couldn't parse object ¯\_(ツ)_/¯
}
Comment on lines +17 to +19
Copy link
Copy Markdown

@echolayer-dev-karl echolayer-dev-karl Bot Apr 24, 2024

Choose a reason for hiding this comment

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

The catch block on lines 7-9 should log the error to avoid silent failures during JSON stringification of the meta object. Consider using console.error or the winston logger itself to record the error.

}
}
return message;
Expand Down Expand Up @@ -40,7 +46,7 @@ const options = {
environment: process.env.ENVIRONMENT || "development",
normalizeDepth: 10,
},
level: "error",
level: process.env.SENTRY_LOG_LEVEL || "error",
Copy link
Copy Markdown

@echolayer-dev-karl echolayer-dev-karl Bot Apr 24, 2024

Choose a reason for hiding this comment

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

Ensure that the environment variable SENTRY_LOG_LEVEL is properly documented and validated to prevent invalid log levels from being used, which could cause runtime issues.

format: sentryFormat(),
};

Expand Down
39 changes: 27 additions & 12 deletions codex-vscode/src/services/git.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,9 @@ export class GitService implements Singleton {
const repositories: Array<VSCodeRepository> = (
await Promise.all(
this.gitAPI.repositories.map(async (repository) => {
if (repository?.state?.remotes?.[0]?.fetchUrl) {
const vscodeRepository: VSCodeRepository = GitService.toVSCodeRepository(repository);
const remote = GitService.getRemote(repository);
if (remote?.fetchUrl) {
const vscodeRepository: VSCodeRepository = GitService.toVSCodeRepository(repository, remote);
Comment on lines +52 to +54
Copy link
Copy Markdown

@echolayer-dev-karl echolayer-dev-karl Bot Apr 24, 2024

Choose a reason for hiding this comment

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

The refactoring to use GitService.getRemote(repository) improves code readability by abstracting the logic for obtaining the remote. However, ensure that the toVSCodeRepository method is updated to accept the new remote parameter as it now requires two arguments.

await this.localRepositoryMapManager.set(vscodeRepository.origin, vscodeRepository.directory);
return vscodeRepository;
}
Expand All @@ -68,9 +69,9 @@ export class GitService implements Singleton {

async getRepositoryAt(uri: vscode.Uri): Promise<VSCodeRepository | undefined> {
const repository = this.gitAPI.getRepository(uri);
const fetchOrigin = GitService.getFetchOrigin(repository);
if (fetchOrigin) {
const vscodeRepository: VSCodeRepository = GitService.toVSCodeRepository(repository);
const remote = GitService.getRemote(repository);
if (remote) {
const vscodeRepository: VSCodeRepository = GitService.toVSCodeRepository(repository, remote);
Comment on lines 71 to +74
Copy link
Copy Markdown

@echolayer-dev-karl echolayer-dev-karl Bot Apr 24, 2024

Choose a reason for hiding this comment

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

The refactoring from getFetchOrigin to getRemote and the corresponding parameter change in toVSCodeRepository method improves clarity and maintainability by using a more descriptive method name and passing the relevant Remote object directly.

return vscodeRepository;
} else {
try {
Expand Down Expand Up @@ -110,11 +111,8 @@ export class GitService implements Singleton {
}
}

static toVSCodeRepository(repository: Repository): VSCodeRepository | undefined {
const fetchOrigin = GitService.getFetchOrigin(repository);
if (!fetchOrigin) return;

const parsedRemoteGitUrl = gitUrlParse(fetchOrigin.fetchUrl);
static toVSCodeRepository(repository: Repository, remote: Remote): VSCodeRepository | undefined {
const parsedRemoteGitUrl = gitUrlParse(remote.fetchUrl);
Comment on lines 113 to +115
Copy link
Copy Markdown

@echolayer-dev-karl echolayer-dev-karl Bot Apr 24, 2024

Choose a reason for hiding this comment

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

The refactoring of toVSCodeRepository to take a Remote object directly simplifies the method and removes the need for a null check on fetchOrigin. Ensure that all calls to this method are updated to pass the Remote object as the second argument.

if (parsedRemoteGitUrl) {
return {
name: parsedRemoteGitUrl.name,
Expand All @@ -124,7 +122,24 @@ export class GitService implements Singleton {
}
}

private static getFetchOrigin(repository: Repository): Remote | undefined {
return repository?.state?.remotes?.find((remote) => remote.name === "origin" && remote.fetchUrl);
/**
* Retrieves a remote repository; ideally returns the Origin, but if not, returns a fallback
* @param repository
* @returns Remote | undefined
*/
private static getRemote(repository: Repository): Remote | undefined {
let originRepo = repository?.state?.remotes?.find((remote) => remote.name === "origin" && remote.fetchUrl);
if (originRepo) {
return originRepo;
}

logger.info(`Could not find repo origin - gitService.getFetchOrigin`, {
Copy link
Copy Markdown

@echolayer-dev-karl echolayer-dev-karl Bot Apr 24, 2024

Choose a reason for hiding this comment

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

The log message in line 17 references a method getFetchOrigin that has been renamed to getRemote. Update the log message to reflect the correct method name to avoid confusion during debugging.

repositoryRemotes: repository?.state?.remotes,
});
// We couldn't find a repo w/ Origin as it's name, so we'll naively take the first one
originRepo = repository?.state?.remotes && repository?.state?.remotes[0];
Copy link
Copy Markdown

@echolayer-dev-karl echolayer-dev-karl Bot Apr 24, 2024

Choose a reason for hiding this comment

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

Consider adding a null check for repository?.state?.remotes before accessing the first element in line 21 to prevent potential runtime errors if remotes is undefined.

if (originRepo?.fetchUrl) {
return originRepo;
}
}
}