-
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
GTT Parser #52
GTT Parser #52
Conversation
for table in tables: | ||
for td_element in table.find_all("td"): | ||
if "Planned Work Notification" in td_element.text: | ||
data["maintenance_id"], status = td_element.text.strip().split(": ")[1].split(" - ") |
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.
That's a pretty gnarly bit of string manipulation there. Would a regex be a cleaner way to work with this instead?
data["start"] = 0 | ||
data["end"] = 1 |
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.
This seems strange/odd to me. Does a cancelled notification not specify the original start/end time or something?
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.
maybe a comment about the hack?
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.
Sorry needs a comment. When the cancelled email notification comes in, it doesnt have a start and end time so I set it to 0 and 1 as I dont know the dates.
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 this only an issue when we get a Cancelled notification and we hadn't previously gotten a Scheduled notification for this maintenance? I'd think that in the case where it's an update to a previous notification we should be able to preserve the timestamps from the original notification.
if num_columns: | ||
data["circuits"] = [] | ||
cells = table.find_all("td") | ||
for idx in range(0, len(cells), num_columns): # pylint: disable=unused-variable |
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.
Why do we have idx
here, as it looks completely unused?
HtmlParserGTT1, | ||
Path(dir_path, "data", "gtt", "gtt2.html"), | ||
Path(dir_path, "data", "gtt", "gtt2_result.json"), | ||
), |
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.
Missing gtt3
test?
@@ -9,6 +9,7 @@ | |||
GenericProvider, | |||
Cogent, | |||
EUNetworks, | |||
GTT, |
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.
could you update Readme and Changelog?
( | ||
HtmlParserGTT1, | ||
Path(dir_path, "data", "gtt", "gtt2.html"), | ||
Path(dir_path, "data", "gtt", "gtt2_result.json"), |
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.
why not using gtt3?
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.
LGTM (before seeing the tests pass)
Parser added for GTT provider.