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

Creation dummy node #34

Closed
wants to merge 3 commits into from
Closed

Creation dummy node #34

wants to merge 3 commits into from

Conversation

VinciGit00
Copy link
Collaborator

No description provided.

@VinciGit00 VinciGit00 requested a review from lurenss March 3, 2024 11:33
Copy link

github-actions bot commented Mar 3, 2024

Dependency Review

✅ No vulnerabilities or license issues found.

Scanned Manifest Files

Copy link
Member

@lurenss lurenss left a comment

Choose a reason for hiding this comment

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

I have some suggestions for the prints, parameter name and node comments, those would avoid misunderstanding from the final users IMO

KeyError: If the 'url' key is not found in the state, indicating that the
necessary information to perform the operation is missing.
"""
print("---LOADING HTML CODE---")
Copy link
Member

Choose a reason for hiding this comment

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

LOADING TEXT CODE, because can read generic text not only html

text = open("text_example.txt", "r", encoding="utf-8")

# define the nodes for the graph
fetch_html_node = TextNode("fetch_html")
Copy link
Member

Choose a reason for hiding this comment

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

It isn't supposed to read only html but general text, I would change to fetch_text_node

node_name (str): The unique identifier name for the node. This name is used to
reference the node within the graph.
node_type (str, optional): The type of the node, limited to "node" or
"conditional_node". Defaults to "node".
Copy link
Member

Choose a reason for hiding this comment

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

I would rewrite the documentation for this node in order to make as cleas as possible that takes general text, the parameter url for this use case could cause misunderstanding, i would change to file

@VinciGit00 VinciGit00 closed this Mar 3, 2024
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