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

Refactor Langchain file #175

Closed
wants to merge 5 commits into from

Conversation

DogukanGun
Copy link

refactor langchain file.
This pull request was created for https://app.gib.work/bounties/0b78790e-6d48-4ec3-b67f-46c96dac6b7d in an attempt to solve a bounty #173 . Payment for the bounty is immediately sent to the contributor after merge.

@DogukanGun
Copy link
Author

Hi @thearyanag , my pr is ready for review. Thanks!

@thearyanag
Copy link
Member

hey @DogukanGun - can you rebase it once? looks like it does not have the latest changes from main branch + during rebase don't commit the docs pls

@thearyanag
Copy link
Member

thearyanag commented Jan 9, 2025

hey @DogukanGun -we more wanted that each tool and action present under src/langchain and src/actions should be grouped into proper folders given the protocol name

  1. spit the langchain/index.ts into multiple folders, depending on protocol name
  2. divide the actions under src/actions into multiple folders, depending on protocol name
  3. divide the tools under src/tools into multiple folders, depending on protocol name

re-writing the issue to be more clear

rewrote the issue to avoid any confusion, thanks

@DogukanGun DogukanGun reopened this Jan 10, 2025
@DogukanGun
Copy link
Author

DogukanGun commented Jan 10, 2025

Hi @thearyanag , thank you for the review! I have made the changes in the langchain folder as per your suggestions. Once you approve the approach for the changes in langchain, I will proceed to update the tool and action folders as well.

@DogukanGun
Copy link
Author

Hi @thearyanag, is this implementation correct ? If it is, then I can finish actions folder also.

@thearyanag
Copy link
Member

hey @DogukanGun - the direction is absolutely correct !

@DogukanGun
Copy link
Author

Hi @thearyanag , thanks for your review. I finished the implementation and the pr is ready for review.

@DogukanGun DogukanGun closed this Jan 11, 2025
@DogukanGun DogukanGun closed this Jan 11, 2025
@DogukanGun DogukanGun reopened this Jan 12, 2025
@DogukanGun
Copy link
Author

Hi @thearyanag , I noticed that the accepted PR seems quite similar to mine, with the main difference being the index files in each subfolder. I was wondering if there were any other differences that led to that PR being merged instead? I’d appreciate any feedback you can provide! 😊

@thearyanag
Copy link
Member

hey @DogukanGun , both were quite similar indeed but

  1. it was throwing error while running locally for me
  2. the index file indeed provided a better folder structure,

@DogukanGun
Copy link
Author

hey @DogukanGun , both were quite similar indeed but

  1. it was throwing error while running locally for me
  2. the index file indeed provided a better folder structure,

Thank you for the feedback! 😊 I understand now, and I’ll focus on working on other PRs.

@DogukanGun DogukanGun closed this Jan 12, 2025
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.

2 participants