-
Notifications
You must be signed in to change notification settings - Fork 1k
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
[AssociatedPressNewsBridge] Add bridge #1475
Conversation
LGTM, a very complete bridge. |
There's a bug on this article that add an image that don't exist Raw data from API about that article:
|
@csisoap, I've pushed commits that fix this and a few other issues. |
Awesome, thank. |
.@csisoap, I've pushed commits to fix these issues. |
@VerifiedJoseph the error is happening again. I try look into the API data and discover this cause error.
I think those article items have their |
I see you have pushed commit. Thank you |
Is this PR still relevant? |
Yes, but it needs a rebase, or the site changed the format. |
First context seems to work fine, second has an issue:
|
Your query string is missing the context parameter. Should be like: |
Hi, Yeah, I saw that. It's interesting, this has never been an issue before. The output was from my automated testing, just using the bridgelist to generate the context will do it. But it's interesting that I've never seen this before... All other bridges do not care if the context is not provided as a parameter because they derive the context within the bridge. Not saying it's bad, its just different. Couldnt look into the bridge detail yet, will do so soon |
Tested and works. Can you add a limit option? |
Pull request artifacts
|
This works fine. The limit is at 15, so I think its a good limit for the amount of updates. LGTM, merging |
Adds bridge for Associated Press website. Closes #1463