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

[ReutersBridge] Add New Bridge #1653

Merged
merged 10 commits into from
Jan 29, 2021
Merged

Conversation

hollowleviathan
Copy link
Contributor

@hollowleviathan hollowleviathan commented Jul 13, 2020

Adds bridge requested in #1618

@hollowleviathan hollowleviathan changed the title [ReutersBridge] Add New Bridge (#1618) [ReutersBridge] Add New Bridge Jul 13, 2020
@csisoap
Copy link
Contributor

csisoap commented Jul 14, 2020

Bridge is working perfectly.
Thank you.
But it seem "Pictures" section isn't working. I got blank page on that section.

Query string: action=display&bridge=Reuters&feed=pictures&format=Html

@somini
Copy link
Contributor

somini commented Jul 14, 2020

See #1651 (comment)

@csisoap
Copy link
Contributor

csisoap commented Jul 17, 2020

image

It would be great if it can get the article from "Editor's Highlight" in the above picture. It seem they are different than the "story" type.

@Spraynard
Copy link

Spraynard commented Dec 25, 2020

Are there any issues regarding merging this PR? I can help get everything ready for merging if the project needs it

@hollowleviathan
Copy link
Contributor Author

hollowleviathan commented Dec 26, 2020

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"

@Spraynard
Copy link

Alright thank you for giving me an update! I'll see if there's anything that can be added

@csisoap
Copy link
Contributor

csisoap commented Dec 27, 2020

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"

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,...).

@hollowleviathan
Copy link
Contributor Author

Can i ask where you get that bug from?

Observation from using this PR myself.

I think the advantage of this using direct API is that you get all info of that article (id, timestamp, article's content,...).

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?

@Spraynard
Copy link

Spraynard commented Dec 27, 2020

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.

Just briefly looking while writing this comment, our bridge is pulling from https://reuters.com whereas #1651 bridge data is coming from https://uk.reuters.com. We might have to put a selector in our bridge to merge in uk.reuters data. Sorry I'm trying to remember what is actually happening and I was ignorant

@Spraynard
Copy link

This linked pull request to the reuters branch of @hollowleviathan's repo should combine the functionality between this PR and #1651.

Selecting US will present stories and link back to reuters.com, whereas selecting UK will link back to uk.reuters.com.

hollowleviathan and others added 6 commits December 30, 2020 17:21
News Feed types now selectable through a list parameter

JSON return is now being parsed correctly

Stories being gathered in a stepwise process
[ReutersBridge] Add all article from 'Editor\'s Highlight' to the feed, more categories, author name, full article text.
Adding region selection to ReutersBridge
@csisoap
Copy link
Contributor

csisoap commented Jan 5, 2021

The PR is now ready to be merged. It's just need to get merge with PR #1651.

@Spraynard
Copy link

@csisoap Sorry I am unsure what you mean by merge with the other PR. I looked at what #1651 was doing and saw that it was looking at stories from uk.reuters.com and so I added the region contexts. I think that we can merge this and have to ask #1651 to close their pull request 😔

@csisoap
Copy link
Contributor

csisoap commented Jan 10, 2021

@csisoap Sorry I am unsure what you mean by merge with the other PR. I looked at what #1651 was doing and saw that it was looking at stories from uk.reuters.com and so I added the region contexts. I think that we can merge this and have to ask #1651 to close their pull request 😔

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.

@Spraynard
Copy link

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

@csisoap
Copy link
Contributor

csisoap commented Jan 11, 2021

@Spraynard it seem there only @em92 doing review for PRs right now.

Copy link
Contributor

@em92 em92 left a comment

Choose a reason for hiding this comment

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

Comment on lines 31 to 79
'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',
)
)
Copy link
Contributor

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.

Copy link
Contributor

@csisoap csisoap Jan 19, 2021

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.

@csisoap
Copy link
Contributor

csisoap commented Jan 17, 2021

UK and US feeds look equal.
https://feed.eugenemolotov.ru/pr1653/?action=display&bridge=Reuters&context=United+States&feed_us=tech&format=Html
https://feed.eugenemolotov.ru/pr1653/?action=display&bridge=Reuters&context=United+States&feed_uk=tech&format=Html

Is that expected?

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.
@em92
Copy link
Contributor

em92 commented Jan 22, 2021

Alphabetical order is required. Hard to find category, that user could be interested in.

Снимок экрана_2021-01-22_10-01-03

@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?

@csisoap
Copy link
Contributor

csisoap commented Jan 23, 2021

Hi @em92
The reason to remove U.K context was that Reuters merged several edition, includes U.K, into one global site so i personally think it would not necessary to use U.K context anymore.
I have added all feeds that specfic to U.K edition into U.S context.

You can find their statement here.

Sorting selection  of news feed values
@Spraynard
Copy link

News feed items have been sorted with this pull request. Once we get that merged we should fit the sorted feed items criteria.

@em92
Copy link
Contributor

em92 commented Jan 25, 2021

You can find their statement here.

Fair enough.

News feed items have been sorted with this pull request. Once we get that merged we should fit the sorted feed items criteria.

@hollowleviathan waiting for you to push mentioned PR.

@csisoap
Copy link
Contributor

csisoap commented Jan 29, 2021

Pinging @hollowleviathan just in case he forgot.

@hollowleviathan
Copy link
Contributor Author

Sorry about the delay, merged now.

@em92 em92 merged commit 43b7621 into RSS-Bridge:master Jan 29, 2021
@em92
Copy link
Contributor

em92 commented Jan 29, 2021

gj you all!

@hollowleviathan hollowleviathan deleted the reuters branch February 3, 2021 19:40
@Spraynard
Copy link

Amazing!!!!!

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.

None yet

5 participants