-
Notifications
You must be signed in to change notification settings - Fork 24
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
New library design #53
Conversation
98aa452
to
c7a4cf5
Compare
circuit_maintenance_parser/data.py
Outdated
|
||
|
||
class NotificationData(BaseModel, extra=Extra.forbid): | ||
"""Base class for Notificaiton Data types.""" |
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.
Can this docstring please be detailed a bit more? Maybe with examples of what is meant by "notification data", how this can store one or several DataParts, etc.?
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.
sure, this PR is not finished (documentation, tests, etc.). I will take this into account after the initial review of the design
Co-authored-by: Glenn Matthews <[email protected]>
try: | ||
maintenances_data.append(Maintenance(**self.combined_maintenance_data)) | ||
except ValidationError as exc: | ||
raise ProviderError("Not enough information available to create a Maintenance notification.") from exc |
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 think I suggested ProviderError
in a previous review comment, but actually, should this be ProcessorError
now?
@@ -74,52 +52,45 @@ class ICal(Parser): | |||
Reference: https://tools.ietf.org/html/draft-gunter-calext-maintenance-notifications-00 | |||
""" | |||
|
|||
_data_type = "text/calendar" | |||
_data_types = ["text/calendar", "ical", "icalendar"] |
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.
Perhaps I missed it, but is it documented what each element of _data_types
represents?
@@ -152,35 +120,29 @@ def process(self) -> Iterable[Maintenance]: | |||
class Html(Parser): | |||
"""Html parser.""" | |||
|
|||
_data_type = "text/html" | |||
_data_types = ["text/html", "html"] |
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.
is it documented what each element of _data_types represents?
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.
They are just arbitrary data types that you expect to match... so following your point, we could either document it or maybe create some ENUM that could help understanding them
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.
Makes sense. I'm sure documenting would suffice. No need to change type.
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.
Honestly it's a bit hard to follow with all the refactoring and reorganization of code but I think this appears reasonable. Let's get it merged so that we can start actually trying out some new parsers with the combined data.
Co-authored-by: Glenn Matthews <[email protected]>
TODO
The main idea behind this refactor is to make the library more "autonomous" to get all the information that it needs to create notifications. The main change is that, before, a notifications was parsed only by ONE parser each time, now we could combine multiple parsers to get all the information needed.
The workflow would look like:
Provider
(if no provider type, a standard Icalendar one is offered by default).Data
to be parsedIn this case we are using the simples
init
, but aninit_from_email
could be used to call the library using a standard Email representation. This way, the library is not closely tangled with a specific data source, and can be reused in multiple use cases, but at the same time offers all the data structure processing, so the library user don't need to take care of.Data
using theProvider
processor (that is composed by multiple combined parsers)