SDNTB-604 Move NTB related feed parser and formatter from planning to NTB repo #1363
Conversation
| @@ -117,7 +121,7 @@ def _update(self, provider, update): | |||
| content = io.BytesIO(attachment) | |||
| res = process_file_from_stream(content, part.get_content_type()) | |||
| file_name, content_type, metadata = res | |||
| if isinstance(parser, NTBEventXMLFeedParser): | |||
| if NTBEventXMLFeedParser and isinstance(parser, NTBEventXMLFeedParser): | |||
There was a problem hiding this comment.
Instead of attempting to import the NTBEventXMLFeedParser and testing isinstance, I think it might be better to use something similar to can_parse of the registered feed parser itself. This way these 3 usages for both NTBEventXMLFeedParser and IcsTwoFeedParser can be removed and move the logic into the parser themselves.
There was a problem hiding this comment.
The logic we have right now in event_file_service.py, event_http_service.py and event_email_service.py checks a class anyway. I don't see a way how a method in a parser can help here. We can avoid attempting to import NTBEventXMLFeedParser and import 'IcsTwoFeedParser' by checking a class name if parser.__class__.__name__ == 'NTBEventXMLFeedParser' which is IMO a less reliable.
Can you provide me an example of how you see a method similar to can_parse ?
There was a problem hiding this comment.
Hi @MarkLark86 , any update on it ⬆️ ?
There was a problem hiding this comment.
You can use python's getattr to determine if the parser has a specific method defined, otherwise use the default. The purpose is to remove all specific NTB/ICS code from these services.
You could use something like the following:
if getattr(parser, 'parse_email'):
parser.parse_email(....)
else:
logger.warn('Ingesting event with unknown parser'
new_items.append(parser.parse(data, provider)And then implement something similar to parse_file and parse_http.
This will make it easier to maintain the code by separating out the implementation of the parser from the service entirely.
There was a problem hiding this comment.
@MarkLark86 check it, please.
Сorresponding сommit for NTB superdesk/superdesk-ntb@9d21317
P.S. We must restrict parsers per feeding service again. It was done some time ago, but it is time to do it again. For example, I don't think that all parsers should be available when EventHTTPFeedingService selected and etc. Anyway, this is out of the scope of this pr.
This PR goes side by side with superdesk/superdesk-ntb#344.
PR for NTB is broken because it has planning 1.7 as a dependency, instead, I've triggered a test instance superdesk/superdesk-ntb#345 (https://ntbpr-345.test.superdesk.org).
NOTE: I was able to ingest and output events on a test instance https://ntbpr-345.test.superdesk.org