Skip to content

refactor: tool registration #8

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

Merged
merged 7 commits into from
Apr 8, 2025
Merged

refactor: tool registration #8

merged 7 commits into from
Apr 8, 2025

Conversation

nirinchev
Copy link
Collaborator

@nirinchev nirinchev commented Apr 8, 2025

Refactor the way tools are defined and registered:

  • Creates a base class for tools that encapsulates the execution logic along with the metadata
  • Move the Atlas tools implementations to their own classes inside tools/atlas
  • Register all tools in tools/atlas/index.ts and tools/mongodb/index.ts

Additionally, it adds a few QOL improvements:

  • Enable sourcemaps
  • Add a launch profile for debugging the server with vscode
  • Remove npx calls from package.json

@nirinchev nirinchev requested review from fmenezes, gagik and blva April 8, 2025 12:28
package.json Outdated
"check:format": "npx prettier -c .",
"reformat": "npx prettier -- --write ."
"check:lint": "eslint .",
"check:format": "npm run prettier -c .",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
"check:format": "npm run prettier -c .",
"check:format": "prettier -c .",

I think this should work

Copy link
Collaborator

Choose a reason for hiding this comment

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

or simply rebase on main

@fmenezes fmenezes changed the title Refactor the tool registration refactor: tool registration Apr 8, 2025
fmenezes
fmenezes previously approved these changes Apr 8, 2025
Copy link
Collaborator

@fmenezes fmenezes left a comment

Choose a reason for hiding this comment

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

LGTM, left one question

import { Transport } from "@modelcontextprotocol/sdk/shared/transport.js";
import { z, ZodOptional, ZodString } from "zod";
import { ApiClient, AtlasCluster } from "./client.js";
import { ApiClient } from "./client.js";
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we perhaps move client and all API stuff to AtlasToolBase? I think only atlas will use it

Copy link
Collaborator

Choose a reason for hiding this comment

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

telemetry will use it I think, which won't be atlas

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hm... not super familiar with how we're going to be doing telemetry, so will leave it as is for now, but we can definitely move it over if it turns out it's not needed for telemetry.

* main:
  ci: publish to npm (#9)

# Conflicts:
#	dist/client.js
#	dist/config.js
#	dist/index.js
#	dist/server.js
#	dist/state.js
#	package.json
@fmenezes
Copy link
Collaborator

fmenezes commented Apr 8, 2025

@nirinchev make sure to delete dist folder on your branch before merging

@nirinchev nirinchev merged commit bdee527 into main Apr 8, 2025
2 checks passed
@nirinchev nirinchev deleted the ni/server-restructure branch April 8, 2025 14:17
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.

4 participants