-
Notifications
You must be signed in to change notification settings - Fork 1.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
[ReutersBridge] Add New Bridge #1653
Conversation
Bridge is working perfectly. Query string: |
See #1651 (comment) |
581fbfc
to
0e41636
Compare
b600c35
to
d4a689d
Compare
d4a689d
to
f959043
Compare
Are there any issues regarding merging this PR? I can help get everything ready for merging if the project needs it |
The primary concern from the RSS-Bridge team seems to be checking if #1651 does anything this PR doesn't and incorporating it. Since that PR is a scrape of the UK reuters site, I'm daunted by figuring out how to understand what advantages, if any, it has over using the direct API. I also notice there's a bug where this PR has some tag text gunk at the end of articles "for-phone-onlyfor-tablet-portrait-upfor-tablet-landscape-upfor-dekstop-upfor-wide-desktop-up" |
Alright thank you for giving me an update! I'll see if there's anything that can be added |
Can i ask where you get that bug from? I think the advantage of this using direct API is that you get all info of that article (id, timestamp, article's content,...). |
Observation from using this PR myself.
Yes, I think using the direct API is superior and should get all of the news items that a scrape gets and more, but how do you prove that? |
From what I understand I don't think people are arguing to keep the scraping method, I just think people are wondering if there's any more information obtained from the scrape, (or even a different source of information) than what is being obtained in this bridge.
|
This linked pull request to the Selecting US will present stories and link back to |
News Feed types now selectable through a list parameter JSON return is now being parsed correctly Stories being gathered in a stepwise process
…d more categories
[ReutersBridge] Add all article from 'Editor\'s Highlight' to the feed, more categories, author name, full article text.
df9979d
to
da901cc
Compare
Adding region selection to ReutersBridge
The PR is now ready to be merged. It's just need to get merge with PR #1651. |
Yeah, my fault. Sorry about that, i'm terrible at English :( About asking #1651 , i tried to ask him to check this PR but i haven't received any response. |
You're fine :) English is a dumb language anyway. Hmmm okay. Well I don't know who reviews these pull requests but since this one seems to be more robust and is working fine I think it can be merged. I don't know if #1651 is viable anymore |
@Spraynard it seem there only @em92 doing review for PRs right now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bridges/ReutersBridge.php
Outdated
'United Kingdom' => array( | ||
'feed_uk' => array( | ||
'name' => 'News Feed', | ||
'type' => 'list', | ||
'title' => 'Feeds from Reuters U.K edition', | ||
'values' => array( | ||
'Tech' => 'tech', | ||
'Wire' => 'wire', | ||
'Business' => 'business', | ||
'World' => 'world', | ||
'Politics' => 'politics', | ||
'Science' => 'science', | ||
'Energy' => 'energy', | ||
'Aerospace and Defense' => 'aerospace', | ||
'Special Reports' => 'special-reports', | ||
'Top News' => 'home/topnews', | ||
'Markets' => 'markets', | ||
'Sports' => 'sports', | ||
'UK' => 'uk', | ||
'Entertainment' => 'entertainment', | ||
'Environment' => 'environment', | ||
'Lifestyle' => 'lifestyle', | ||
) | ||
) | ||
), | ||
'United States' => array( | ||
'feed_us' => array( | ||
'name' => 'News Feed', | ||
'type' => 'list', | ||
'title' => 'Feeds from Reuters U.S/International edition', | ||
'values' => array( | ||
'Tech' => 'tech', | ||
'Wire' => 'wire', | ||
'Health' => 'health', | ||
'Business' => 'business', | ||
'World' => 'world', | ||
'Politics' => 'politics', | ||
'Science' => 'science', | ||
'Lifestyle' => 'life', | ||
'Energy' => 'energy', | ||
'Aerospace and Defense' => 'aerospace', | ||
'Special Reports' => 'special-reports', | ||
'China' => 'china', | ||
'Top News' => 'home/topnews', | ||
'Markets' => 'markets', | ||
'Sports' => 'sports', | ||
'USA News' => 'us', | ||
) | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest to extract common catogories to some array. Then use array_merge to add non-common ones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, i tried to split up common categories to some array and when i tried to merge, the compiler doesn't happy with me as const don't allow expression. Same happen with private. I don't know how to merge two array into one giant parameter as you saw in the code. There are spread operator which can use to merge but it's on PHP 7.4 which may failed some test for older version.
I just hope this bridge will soon to be merged.
You have to change the context param to United Kingdom on the second link to see what different between two feeds. Sometimes they equal each other, but if you look into other feed like World, Top News, you'll see the order between is different. |
- Health feed in the U.S context now using different feed. - Bridge now support using a unique feed ID instead of normal name. - Remove U.K context. - Add some new feeds.
Alphabetical order is required. Hard to find category, that user could be interested in. @Spraynard and @hollowleviathan. There was a discussion about UK and US versions of reuters. Is it fine, that latest changes remove separated US and UK feeds? |
Sorting selection of news feed values
News feed items have been sorted with this pull request. Once we get that merged we should fit the sorted feed items criteria. |
Fair enough.
@hollowleviathan waiting for you to push mentioned PR. |
Pinging @hollowleviathan just in case he forgot. |
Updating ReutersBridge
Sorry about the delay, merged now. |
gj you all! |
Amazing!!!!! |
Adds bridge requested in #1618