-
Notifications
You must be signed in to change notification settings - Fork 2
Feat/disable entrypoint for base context 2 #121
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
Conversation
Base Context를 실행할 때도 executionDelay와 runningCodes가 보고되는 문제를 해결했습니다. - `YaksokSession`의 `BASE_CONTEXT_SYMBOL`을 public getter로 변경하고, `Block` 노드에서 현재 실행 중인 코드가 Base Context인지 확인하여 `executionDelay`와 `runningCode` 보고를 건너뛰도록 수정했습니다.
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.
Summary of Changes
Hello @rycont, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request primarily introduces a new 'base context' feature, allowing certain code executions to bypass standard delays and reporting. This is crucial for optimizing performance in specific scenarios where such overhead is undesirable. Alongside this core functionality, the PR includes comprehensive test coverage for the new feature and updates various package versions to ensure compatibility and leverage the latest dependencies.
Highlights
- Feature: Base Context Handling: Introduced a mechanism to define and identify a 'base context' within the
YaksokSession
class using a newBASE_CONTEXT_SYMBOL
. - Execution Logic Modification: Updated the
Block
class's execution logic to skip applying execution delays and reporting for code running within the identified base context, optimizing performance for background or foundational operations. - Test Coverage: Added a new test case (
test/execution-delay.test.ts
) to specifically verify that execution delays are correctly bypassed for the base context while still applying to the main execution flow. - Version Bumps: Incremented the project version from
2.2.1
to2.2.2
acrosscore
,monaco-language-provider
, andquickjs
packages, along with updating the maindeno.json
anddeno.lock
files to reflect new dependency versions.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command>
or @gemini-code-assist <command>
. Below is a summary of the supported commands.
Feature | Command | Description |
---|---|---|
Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/
folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
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.
Code Review
This pull request introduces a new feature to handle a "base context" in the execution flow, which is a great addition for managing setup code. The implementation is solid, and the new test case effectively validates the desired behavior. I've provided a few suggestions to enhance code maintainability and style consistency by refactoring a repeated condition and aligning a getter's name with common TypeScript conventions.
const isBaseContext = | ||
scope.codeFile?.fileName === | ||
scope.codeFile?.session?.BASE_CONTEXT_SYMBOL |
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 align with common TypeScript style conventions (like the Google TypeScript Style Guide), public properties and getters should use camelCase
. This should be updated to baseContextSymbol
to match the suggested change in YaksokSession
.
const isBaseContext = | |
scope.codeFile?.fileName === | |
scope.codeFile?.session?.BASE_CONTEXT_SYMBOL | |
const isBaseContext = | |
scope.codeFile?.fileName === | |
scope.codeFile?.session?.baseContextSymbol |
if (executionDelay && isMainContext && !isBaseContext) { | ||
await new Promise((r) => setTimeout(r, executionDelay)) | ||
} | ||
|
||
if (child.tokens.length && isMainContext) { | ||
if (child.tokens.length && isMainContext && !isBaseContext) { | ||
this.reportRunningCode(child, scope) | ||
} |
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.
The condition isMainContext && !isBaseContext
is checked twice in this block. To improve readability and reduce redundancy, you can group the logic under a single conditional check.
if (executionDelay && isMainContext && !isBaseContext) { | |
await new Promise((r) => setTimeout(r, executionDelay)) | |
} | |
if (child.tokens.length && isMainContext) { | |
if (child.tokens.length && isMainContext && !isBaseContext) { | |
this.reportRunningCode(child, scope) | |
} | |
if (isMainContext && !isBaseContext) { | |
if (executionDelay) { | |
await new Promise((r) => setTimeout(r, executionDelay)) | |
} | |
if (child.tokens.length) { | |
this.reportRunningCode(child, scope) | |
} | |
} |
readonly #BASE_CONTEXT_SYMBOL = Symbol('baseContext') | ||
public get BASE_CONTEXT_SYMBOL(): symbol { | ||
return this.#BASE_CONTEXT_SYMBOL | ||
} |
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.
Using a private field with a public getter is a great pattern for encapsulation. However, to align with common TypeScript style conventions (e.g., Google's style guide), public properties and getters should use camelCase
. The name BASE_CONTEXT_SYMBOL
uses SCREAMING_SNAKE_CASE
, which is generally reserved for compile-time constants. Renaming this to baseContextSymbol
would improve code consistency. Remember to update its usage in core/node/block.ts
as well.
readonly #BASE_CONTEXT_SYMBOL = Symbol('baseContext') | |
public get BASE_CONTEXT_SYMBOL(): symbol { | |
return this.#BASE_CONTEXT_SYMBOL | |
} | |
readonly #baseContextSymbol = Symbol('baseContext') | |
public get baseContextSymbol(): symbol { | |
return this.#baseContextSymbol | |
} |
f354911
to
0ebc481
Compare
Deploying dalbit-yaksok with
|
Latest commit: |
0ebc481
|
Status: | ✅ Deploy successful! |
Preview URL: | https://22f839c3.yaksok-ts.pages.dev |
Branch Preview URL: | https://feat-disable-entrypoint-for.yaksok-ts.pages.dev |
이번 업데이트의 핵심은 'Base Context'에서 실행되는 코드는 각 파일별 변경 사항은 다음과 같습니다.
요약하자면, 디버깅 편의 기능( |
This pull request introduces a new feature to handle a "base context" in the execution flow, updates the version across multiple packages, and adds a corresponding test to ensure the feature works as expected. The most important changes include the addition of the
BASE_CONTEXT_SYMBOL
to theYaksokSession
class, modifications to the execution logic in theBlock
class, and updates to the test suite to verify the new behavior.New Feature: Base Context Handling
core/session/session.ts
: Introduced a private#BASE_CONTEXT_SYMBOL
in theYaksokSession
class, made accessible via a public getter, to identify the base context. This symbol is used to differentiate the base context from the main context during execution.core/node/block.ts
: Updated the execution logic in theBlock
class to skip execution delays and reporting for the base context by adding a newisBaseContext
condition.Test Suite Enhancements
test/execution-delay.test.ts
: Added a new test, "Execution Delay with Base Context," to verify that execution delays are correctly applied to the main context but not to the base context. This ensures the proper functioning of the new base context feature.Version Updates
2.2.1
to2.2.2
incore/deno.json
,deno.json
,monaco-language-provider/deno.json
, andquickjs/deno.json
to reflect the new feature release. [1] [2] [3] [4]