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

Updated Docker.md file with a small insight + created function for tags creation for news flashs #1129

Open
wants to merge 7 commits into
base: dev
Choose a base branch
from

Conversation

sagivharel
Copy link
Collaborator

@atalyaalon I added the solution for the bug I came across a few weeks ago.

@sagivharel sagivharel changed the title Updated Docker.md file with a small insight Updated Docker.md file with a small insight + created function for tags creation for news flashs Mar 16, 2020
@sagivharel
Copy link
Collaborator Author

@atalyaalon I have created the function and also did the required modification to the table.
@Mano3 I didn't want to mess up your code so I will leave the function implementation for the ynet scrapper to you. Just need to call my function and send it the desired text. (I recommend send it the title and description concatenated together for the best results)

@sagivharel
Copy link
Collaborator Author

CI failure caused by not sending values for parameter tags in ynet scraper function

@atalyaalon
Copy link
Collaborator

atalyaalon commented Mar 20, 2020

@sagivharel wonderful! great work!

A few notes:

  1. Sorry - before I missed your messages - regarding the call of the function insert_new_flash_news in ynet_spider please consult @Mano3 and add this fix to this pr.
  2. I think it's better to add either: boolean columns for the tags OR an array field in postgres. However, let's leave it like this for now and perhaps change it in the future.
  3. We need to add tags of: תאונה חזיתית, התהפכות, תאונה עם הולך רגל, תאונה עם ילד/ה או פעוט/ה, תאונה עם קשיש/ה או אדם/אישה מבוגר/ת
    All of the above is an important information we don't want to miss.
    @Mano3 any comments?

@Mano3
Copy link
Collaborator

Mano3 commented Mar 20, 2020

@sagivharel
Seems excellent overall.
I agree with Atalya about the necessity of age tags and accident type tags.
About adding the function to ynet and walla, I rather you insert it yourself so you would get to know the code better, talk with me while you try and I will help.

@atalyaalon atalyaalon linked an issue Apr 1, 2020 that may be closed by this pull request
@atalyaalon atalyaalon requested a review from Mano3 April 18, 2020 15:48
@atalyaalon
Copy link
Collaborator

@Mano3 just to make sure - you're gonna continue this one right?

@Mano3
Copy link
Collaborator

Mano3 commented Apr 23, 2020

@atalyaalon If that's urgent I rather someone else would finish this at the moment.

@Mano3 Mano3 assigned advak and unassigned DanielleMenu May 16, 2020
@Mano3
Copy link
Collaborator

Mano3 commented Jun 5, 2020

@advak Hi Adva, any progress on this?
Quite a lot of changes were made into news flash scraping code. I suggest talking with @elazarg before any update is made

@elazarg
Copy link

elazarg commented Jun 5, 2020

I think for now we can merge only news_flash_tags.py, and when the refactoring is over, adding tags should be a one liner (and of course upgrade/downgrade)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create tags for news flashes
7 participants