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: Fix linter issues #397

Merged
merged 3 commits into from
Nov 20, 2024
Merged

Conversation

martincik
Copy link
Contributor

@martincik martincik commented Nov 18, 2024

Relates to:

No specific issue linked - appears to be a code quality improvement PR

Risks

Low - Changes are purely related to code style and import organization with no functional changes.

Background

What does this PR do?

This PR addresses linting issues in the codebase by:

  1. Reorganizing and consolidating imports
  2. Removing unused imports
  3. Removing empty catch variable declarations
  4. Fixing import ordering
  5. Removing extra blank lines

What kind of change is this?

Improvements (misc. changes to existing features)

Documentation changes needed?

My changes do not require a change to the project documentation.

Testing

Where should a reviewer start?

Review the changes in the following files:

  • packages/core/src/embedding.ts
  • packages/core/src/generation.ts
  • packages/core/src/parsing.ts

Detailed testing steps

No, automated tests are fine. These changes are purely syntactic and don't affect functionality.

The changes include:

  1. Import reorganization
  2. Removal of unused imports (fs)
  3. Simplifying catch blocks by removing unused error variables
  4. Consistent import ordering

Since these are linter-driven changes, existing tests should cover functionality verification.

@@ -65,7 +65,7 @@ export function parseJsonArrayFromText(text: string) {
if (jsonBlockMatch) {
try {
jsonData = JSON.parse(jsonBlockMatch[1]);
} catch (e) {
} catch {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we log the error with console.error instead of removing this?

@@ -118,7 +118,7 @@ export function parseJSONObjectFromText(
if (objectMatch) {
try {
jsonData = JSON.parse(objectMatch[0]);
} catch (e) {
} catch {
Copy link
Collaborator

Choose a reason for hiding this comment

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

same here and everywhere else

@monilpat
Copy link
Collaborator

LGTM outside of adding console.error for the catch statements

@jkbrooks
Copy link
Contributor

@monilpat @martincik We have a build error, can we address before merge?

1s
20s
59s
6s
0s
3s
Run pnpm run build

> eliza@ build /home/runner/work/eliza/eliza
> bash ./scripts/build.sh

Building package: core

> @ai[1](https://github.com/ai16z/eliza/actions/runs/11891646673/job/33225640865?pr=397#step:8:1)6z/[email protected] build
> tsup --format esm --dts

CLI Building entry: src/index.ts
CLI Using tsconfig: tsconfig.json
CLI tsup v8.3.5
CLI Using tsup config: /home/runner/work/eliza/eliza/packages/core/tsup.config.ts
CLI Target: esnext
CLI Cleaning output folder
ESM Build start
ESM dist/index.js     108.07 KB
ESM dist/index.js.map 25[6](https://github.com/ai16z/eliza/actions/runs/11891646673/job/33225640865?pr=397#step:8:7).62 KB
ESM ⚡️ Build success in 3[7](https://github.com/ai16z/eliza/actions/runs/11891646673/job/33225640865?pr=397#step:8:8)ms
DTS Build start
Error: src/embedding.ts(3,10): error TS2300: Duplicate identifier 'models'.
Error: src/embedding.ts(4,10): error TS2300: Duplicate identifier 'IAgentRuntime'.
Error: src/embedding.ts(4,25): error TS2300: Duplicate identifier 'ModelProviderName'.
Error: src/embedding.ts(4,44): error TS2300: Duplicate identifier 'ModelClass'.
Error: src/embedding.ts([8](https://github.com/ai16z/eliza/actions/runs/11891646673/job/33225640865?pr=397#step:8:9),10): error TS2300: Duplicate identifier 'models'.
Error: src/embedding.ts([10](https://github.com/ai16z/eliza/actions/runs/11891646673/job/33225640865?pr=397#step:8:11),10): error TS2300: Duplicate identifier 'IAgentRuntime'.
Error: src/embedding.ts(10,25): error TS2300: Duplicate identifier 'ModelClass'.
Error: src/embedding.ts(10,37): error TS2300: Duplicate identifier 'ModelProviderName'.

Error: error occurred in dts build
    at Worker.<anonymous> (/home/runner/work/eliza/eliza/node_modules/tsup/dist/index.js:[15](https://github.com/ai16z/eliza/actions/runs/11891646673/job/33225640865?pr=397#step:8:16)41:26)
    at Worker.emit (node:events:507:28)
    at MessagePort.<anonymous> (node:internal/worker:267:53)
    at [nodejs.internal.kHybridDispatch] (node:internal/event_target:827:[20](https://github.com/ai16z/eliza/actions/runs/11891646673/job/33225640865?pr=397#step:8:21))
    at MessagePort.<anonymous> (node:internal/per_context/messageport:[23](https://github.com/ai16z/eliza/actions/runs/11891646673/job/33225640865?pr=397#step:8:24):28)
DTS Build error
Failed to build core
 ELIFECYCLE  Command failed with exit code 1.
Error: Process completed with exit code 1.
0s

@martincik martincik force-pushed the fix/fix-linter-issues branch from 7cc6f58 to d49611b Compare November 20, 2024 06:34
@martincik martincik changed the title Fix linter issues fix: Fix linter issues Nov 20, 2024
@martincik
Copy link
Contributor Author

@jkbrooks Build and lint fixed.

@jkbrooks jkbrooks merged commit 9db1f63 into elizaOS:main Nov 20, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants