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

Verify web #9190

Merged
merged 12 commits into from May 15, 2024
Merged

Verify web #9190

merged 12 commits into from May 15, 2024

Conversation

ZoranPandovski
Copy link
Member

@ZoranPandovski ZoranPandovski commented May 9, 2024

Description

This PR enhances the Web handler with several updates:

  1. Resolves issue #9188.
  2. Updates documentation and README to align with our integrations template.
  3. Introduces additional unit tests.
  4. Refactors numerous helper functions.
  5. Improves code quality through linting and the addition of comments.

Type of change

(Please delete options that are not relevant)

  • 🐛 Bug fix (non-breaking change which fixes an issue)
  • ⚡ New feature (non-breaking change which adds functionality)
  • 📢 Breaking change (fix or feature that would cause existing functionality not to work as expected)
  • 📄 This change requires a documentation update

Additional Media:

  1. Query MindsDB Docs
    Screenshot from 2024-05-10 13-58-07
  2. Query PDF file from URL
    Screenshot from 2024-05-10 13-58-43

Checklist:

  • My code follows the style guidelines(PEP 8) of MindsDB.
  • I have appropriately commented on my code, especially in complex areas.
  • Necessary documentation updates are either made or tracked in issues.
  • Relevant unit and integration tests are updated or added.

@ZoranPandovski ZoranPandovski marked this pull request as draft May 9, 2024 11:21
@ZoranPandovski ZoranPandovski marked this pull request as ready for review May 10, 2024 11:54
tests/unit/handlers/test_web.py Show resolved Hide resolved
@@ -0,0 +1,130 @@
import unittest
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 also add some tests for CrawlerTable? For it's select() operation to be precise? Such as for correctly raising exceptions when a OR operator is included or when an invalid URL is passed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I will add them

tests/unit/handlers/test_web.py Show resolved Hide resolved
docs/integrations/app-integrations/web-crawler.mdx Outdated Show resolved Hide resolved
docs/integrations/app-integrations/web-crawler.mdx Outdated Show resolved Hide resolved
Copy link
Collaborator

@MinuraPunchihewa MinuraPunchihewa left a comment

Choose a reason for hiding this comment

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

@ZoranPandovski Looks like there is a small flake issue to be resolved.

@ZoranPandovski
Copy link
Member Author

@MinuraPunchihewa Fixed, plus few more tests for SELECT

@ZoranPandovski ZoranPandovski merged commit 1dc5130 into main May 15, 2024
13 checks passed
@ZoranPandovski ZoranPandovski deleted the verify-web branch May 15, 2024 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: merged
Development

Successfully merging this pull request may close these issues.

[Bug]: Web handler parse content from limited html element
2 participants