Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Source bing webmaster #335

Merged
merged 17 commits into from
Feb 26, 2024
Merged

Conversation

willi-mueller
Copy link
Collaborator

@willi-mueller willi-mueller commented Jan 26, 2024

Tell us what you do here

  • implementing verified source (please link a relevant issue labeled as verified source)

Relevant issue

issue #310

More PR info

TODO before merge

  • copy unit tests into the test file
  • implement test cases for data loading
  • write README.md
  • @sultaniman obtain an API key for CI and verify a domain, e.g. dlthub.com

@willi-mueller
Copy link
Collaborator Author

willi-mueller commented Jan 26, 2024

Hi @sultaniman, could you please give me feedback on:

  1. how to parse the unix timestamp int into a date if the CI bans import of datetime.date. I didn't have time yet to check how to achieve that with dlt.commons.pendulum. So, if it's trivial for you I'd appreciate a pointer.
  2. What about the doctests in the helpers.py file? Should I cut and paste them into the pytest runner? That's probably better than keeping them there and thus introducing duplicate code. Or is there a way to integrate doctest with pytest? I haven't checked out if this SO answer works for this CI process
  3. If you can obtain an API key from Bing Webmaster and share it with me so that I can implement the test suite that calls the API. If you could verify your dlthub.com domain with Bing that would be great! I don't think you would need to submit one or two pages for indexing because I could find your pages already via bing.com.

Copy link
Contributor

@sultaniman sultaniman left a comment

Choose a reason for hiding this comment

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

Should we also add a README.md?

@willi-mueller
Copy link
Collaborator Author

Should we also add a README.md?

absolutely, I'll type that up together with the integration test suite!
Thank you so much @AstrakhantsevaAA and @sultaniman for your review, I'll work on your points.

@willi-mueller willi-mueller marked this pull request as ready for review February 2, 2024 18:20
@sultaniman sultaniman requested a review from burnash February 8, 2024 16:01
@rudolfix rudolfix added the ci from fork Allows to run tests from PR coming from fork label Feb 12, 2024
@rudolfix rudolfix added ci from fork Allows to run tests from PR coming from fork and removed ci from fork Allows to run tests from PR coming from fork labels Feb 26, 2024
@sultaniman sultaniman merged commit 8bacff5 into dlt-hub:master Feb 26, 2024
13 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
ci from fork Allows to run tests from PR coming from fork
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants