-
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
Turkcell Parser #54
Turkcell Parser #54
Conversation
data["end"] = self.dt2ts(parser.parse(td_elements[idx + 1].text.strip())) | ||
elif "Impact of the maintenance" in td_element.text.strip(): | ||
data["summary"] = td_elements[idx + 1].span.text.strip() | ||
if len(tables) == 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.
if I understand correctly, the Circuit info can be part of the "Impact of the maintenance" section or in a separate table?
maybe, to make this more readable, in this "elif" you could get the p_elements
and then implement the if statement at the end:
if len(tables) == 1 and p_elements:
...
elif len(tables) == 2:
...
15f642b
to
042f8ad
Compare
if "planned" in td_element.text.strip(): | ||
data["status"] = Status["CONFIRMED"] | ||
else: | ||
data["status"] = Status["CONFIRMED"] |
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 have an if/else block that does the same thing in both branches? Is one of these Status values incorrect?
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 understood that he only found the "planned" use case, and he was enforcing the default one, that is the same.
Ideally, we should have some elif
in between when more use cases are added...
is this accurate Kristian?
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.
Yeah that is correct Christian. I need more examples with different status. Will be adding more in the future
Added parser fo turkcell