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

Magentic one skeleton #224

Open
wants to merge 67 commits into
base: magentic-one
Choose a base branch
from

Conversation

Merlinvt
Copy link
Collaborator

Magentic One Skeleton

@AgentGenie
Copy link
Collaborator

Would merge this help to merge other PRs?
If yes, please add simple unit tests, update package dependencies in setup.py, setup_autogen.py, setup_ag2.py.
We can merge this PR then.


@property
def page_content(self) -> str:
return "test_page_content"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please consider all code as production code.
return tmp value is not a good practice.
Can't you just do pass here?

pass

def visit_page(self, path_or_uri: str) -> str:
return "test_visit_page"
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 other places



def test_requests_markdown_browser_address(browser):
assert browser.address == "test_address"
Copy link
Collaborator

Choose a reason for hiding this comment

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

You don't need to test a method if it is not implemented.

# SPDX-License-Identifier: Apache-2.0
#
# Portions derived from https://github.com/microsoft/autogen are under the MIT License.
# SPDX-License-Identifier: MIT
Copy link
Collaborator

Choose a reason for hiding this comment

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

No actual code?

# SPDX-License-Identifier: Apache-2.0
#
# Portions derived from https://github.com/microsoft/autogen are under the MIT License.
# SPDX-License-Identifier: MIT
Copy link
Collaborator

Choose a reason for hiding this comment

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

No actual code?

# SPDX-License-Identifier: Apache-2.0
#
# Portions derived from https://github.com/microsoft/autogen are under the MIT License.
# SPDX-License-Identifier: MIT
Copy link
Collaborator

Choose a reason for hiding this comment

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

No class or actual code?

@AgentGenie
Copy link
Collaborator

The skeleton code is usually used to show the structure of the project and show how the interfaces/classes work together without implement the code.
Currently, this PR does not serve this purpose. Could you think of how to break it into small PRs and have meaningful purpose that we can merge incrementally?

@Merlinvt Merlinvt force-pushed the magentic-one-skeleton branch from 376f986 to 03ca569 Compare January 3, 2025 15:39
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