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

Issue-58: Ensure safety usage of the init_data methods #72

Merged
merged 6 commits into from
Sep 9, 2021

Conversation

chadell
Copy link
Collaborator

@chadell chadell commented Sep 8, 2021

Addresses issue #58
Ensure try/except pattern for any issue found while initializing NotificationData from the library client, returning empty data.
I know it's too open, but because there could be multiple errors I tried to make it the simplest in the code instead of documenting all the potential exceptions each method could raise.

Copy link
Contributor

@glennmatthews glennmatthews left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the purpose of these init_data_* methods? They seem like just really thin wrappers around Notificationdata.init* methods - why would I call these instead of calling those directly?

@chadell
Copy link
Collaborator Author

chadell commented Sep 9, 2021

What's the purpose of these init_data_* methods? They seem like just really thin wrappers around Notificationdata.init* methods - why would I call these instead of calling those directly?

Yes, it is really "thinner" than my initial idea, but the main purpose is to expose a method to the client to avoid it know about the internal library details/classes, so, instead of using NotificationData.init_from_email your could directly call init_from_email and you don't need to now, if you don't want, about NotificaitonData class, but as you say the wrapper is so light that is not adding any other value while it's adding duplicated code/tests.
After considering it, understanding the benefit of the simple method as not worth enough, I would consolidate to the class factory.

glennmatthews
glennmatthews previously approved these changes Sep 9, 2021
tests/unit/test_data.py Outdated Show resolved Hide resolved
circuit_maintenance_parser/data.py Outdated Show resolved Hide resolved
@chadell chadell merged commit 818715d into develop Sep 9, 2021
@chadell chadell deleted the issue-58-handle-exceptions-for-data-init branch September 9, 2021 15:35
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 this pull request may close these issues.

2 participants