-
Notifications
You must be signed in to change notification settings - Fork 15.8k
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
New Trello document loader #4767
New Trello document loader #4767
Conversation
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.
Requesting changes to move configuration information into the initializer.
self.api_token = api_token | ||
|
||
def load( | ||
self, |
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.
@GDrupal Thanks for the contribution!
Document loaders require an argumentless load method.
Could you push the configuration into the initializer?
I've commented on the types of some of the parameters in load
, so we can update the type signatures as well when moving to __ init __
langchain/document_loaders/trello.py
Outdated
self, | ||
board_name: str, | ||
card_filter: Optional[str] = "all", | ||
include_card_name: Optional[bool] = True, |
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.
Bools shouldn't have an optional since we only care about True
and False
, not about a None
value. (At least in this case)
include_card_name: Optional[bool] = True, | |
include_card_name: bool = True, |
langchain/document_loaders/trello.py
Outdated
include_card_name: Optional[bool] = True, | ||
include_comments: Optional[bool] = True, | ||
include_checklist: Optional[bool] = True, | ||
extra_metadata: Optional[List[str]] = [ |
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.
Sequence
to denote immutability and assign tuple as default value rather than list. No need for an optional.
extra_metadata: Optional[List[str]] = [ | |
extra_metadata: Sequence[str] = ("due_date", "labels", "list", "is_closed") |
@@ -0,0 +1,124 @@ | |||
"""Loader that loads cards from Trello""" |
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.
Do you have any sample data (can be fake data) to help test that the massaging code inside the loader is correct?
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.
Thanks for you feedback!
I updated most of it already. Now, about the fake data....
what do you need exactly?
Are you testing loaders (with external Api request) with mockups endpoints?
Asking because I could not find examples in the codebase.
In this case we have 2 abstraction layers the actual trello api and the objects returned by py-trello, that last one is the one we interact directly from langchain.
We could create a Trello "Lang Chain Test" board from a dummy email, since the free plan gives you api access too.
But I guess you will have to keep those safe, langchain maintainers side.
No sure if that is even possible for your current test setup.
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.
Ideally we could test everything that follows usage of the trello client.
The client can be patched, to have board.get_cards
return a fixture
cards = board.get_cards(card_filter=self.card_filter) # <-- from a fixture instead of the web
A fixture would allow testing all the code that follows that statement, without having to rely on an internet connection or configuring trello accounts etc.
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.
Just attached a comment in the main methods that retrieve data. Each comment has a JSON example of the data structure that method returns.
I hope that's enough. The object from py-trello are quite obscure, I just simplified them for this use case.
Thanks @GDrupal looking mostly good -- we only need to change the location of configuration to be consistent with the loader interface |
@GDrupal with some sample card data, a maintain will be able to merge this change in. We'll refactor the initializer to accept the client and create a classmethod that allows instantiation from api keys |
@GDrupal apologies for the delay -- I'll try to merge in 1-2 days trying to set some time aside to write the tests. However, if you're familiar with unit test library and how to use mock/patch or use the responses library to get the fixture working feel free to add the tests -- that'll be helpful :) |
@eyurtsev |
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.
@eyurtsev Added unit test cases! let me know if you see something wrong. The Trello objects are quite "weird", it took me some "creative" approach to get it working.
langchain/document_loaders/trello.py
Outdated
for item in checklist.items | ||
] | ||
) | ||
text_content += f"\n{checklist.name}\n" + "\n".join(items) |
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.
guessing only new items should be part of the join? otherwise you'll be reading all the items added from a previous checklists
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.
Good catch!
I saw that you did a little refactor, thanks!
thanks @GDrupal! will mention this feature on twitter, happy to tag you if there's a twitter handle you wanted me to mention! |
Go for it! @musicaoriginal2 |
# Added New Trello loader class and documentation Simple Loader on top of py-trello wrapper. With a board name you can pull cards and to do some field parameter tweaks on load operation. I included documentation and examples. Included unit test cases using patch and a fixture for py-trello client class. --------- Co-authored-by: Dev 2049 <dev.dev2049@gmail.com>
# Added New Trello loader class and documentation Simple Loader on top of py-trello wrapper. With a board name you can pull cards and to do some field parameter tweaks on load operation. I included documentation and examples. Included unit test cases using patch and a fixture for py-trello client class. --------- Co-authored-by: Dev 2049 <dev.dev2049@gmail.com>
Added New Trello loader class and documentation
Simple Loader on top of py-trello wrapper.
With a board name you can pull cards and to do some field parameter tweaks on load operation.
I included documentation and examples.
Included unit test cases using patch and a fixture for py-trello client class.
Thanks in advance for your feedback @eyurtsev.