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

[enrich] Update decorator to add metadeta only for non-empty items #846

Open
animeshk08 opened this issue Apr 14, 2020 · 6 comments · May be fixed by #848
Open

[enrich] Update decorator to add metadeta only for non-empty items #846

animeshk08 opened this issue Apr 14, 2020 · 6 comments · May be fixed by #848

Comments

@animeshk08
Copy link
Contributor

animeshk08 commented Apr 14, 2020

The decorator present in the below method adds metadata to the enriched item even if the item is empty

An empty item may be generated in case the item does not belong to a category that is to be enriched. For e.g in case, the item does not belong to 'issues', 'pull requests' or 'repositories' in the case of github2 enricher.

def get_rich_item(self, item):

An update is also needed here to upload only non-empty documents:

items_to_enrich.append(eitem)

@animeshk08
Copy link
Contributor Author

animeshk08 commented Apr 14, 2020

Proposed solution:

   eitem = func(self, *args, **kwargs)
 + if not eitem:
 +          return eitem
- rich_item = self.get_rich_item(item)
+ if not rich_item:
+       continue
  eitem = self.get_rich_item(item)
- items_to_enrich.append(eitem)
+ if eitem:
+       items_to_enrich.append(eitem)

@animeshk08
Copy link
Contributor Author

animeshk08 commented Apr 14, 2020

Hi, @valeriocos. Let me know if it looks good to you :)

@valeriocos
Copy link
Member

Hi @animeshk08 , your solution looks good, however I don't know if these changes may have side effects. I understand that they shouldn't, but I'm not sure.

@animeshk08
Copy link
Contributor Author

animeshk08 commented Apr 15, 2020

Okay, Let me know how we should move ahead.

I understand that they shouldn't, but I'm not sure.

I think the same.

@valeriocos
Copy link
Member

@animeshk08 , can you prepare a pull request with the suggested changes. I'll perform with it some long-execution tests. Thanks!

@animeshk08
Copy link
Contributor Author

Thanks. I am on it!!

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 a pull request may close this issue.

2 participants