Skip to content

Unified exception when load() is called before save() #67

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

Merged
merged 6 commits into from
Aug 29, 2020

Conversation

rayluo
Copy link
Contributor

@rayluo rayluo commented Aug 8, 2020

Hi Abhi, while pondering #65 , I feel uncomfortable of its introduction of a new None return value, and today I also realize that such a new behavior in the other 3 persistence flavours are inconsistent with FilePersistence. So, in this PR, I'm experimenting a different approach, to NOT introduce the None as a successful return value, but rather raise exception - this time we raise a common IOError. Test cases are not yet added. Please review this proof-of-concept.

UPDATE at 8/28: We settled with a new exception PersistenceNotFound for the scenario described in #64
This PR will replace/close #65, and resolve #64.

@rayluo rayluo requested a review from abhidnya13 August 8, 2020 05:51
@abhidnya13
Copy link
Contributor

Hey Ray, Thanks for getting this perspective, love to see different ways of doing things !
I went through the implementation but I am still not sure why do you feel uncomfortable returning a None value. I would love get more perspective on that. I think we should rather have a conversation before coming up with test cases.

@rayluo
Copy link
Contributor Author

rayluo commented Aug 10, 2020

Good question! The reason I feel uncomfortable of None is still because it is not a valid outcome in the first place. Bogdan happens to also voice his similar concern here. Feel free to ping me in Teams.

@rayluo rayluo force-pushed the handle-error-of-load-before-save branch 2 times, most recently from c91e8ac to f3df576 Compare August 21, 2020 02:14
Copy link
Contributor

@abhidnya13 abhidnya13 left a comment

Choose a reason for hiding this comment

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

It will be also good to have test cases for the newer usecases in Key chain and keyring that we are handling with the help of this PR

@rayluo rayluo force-pushed the handle-error-of-load-before-save branch from b018b0d to 07839e4 Compare August 28, 2020 06:21
@rayluo rayluo marked this pull request as ready for review August 28, 2020 23:42
@abhidnya13 abhidnya13 self-requested a review August 29, 2020 00:06
Copy link
Contributor

@abhidnya13 abhidnya13 left a comment

Choose a reason for hiding this comment

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

LGTM 🚢

@rayluo rayluo merged commit 8605a95 into dev Aug 29, 2020
@rayluo rayluo deleted the handle-error-of-load-before-save branch August 29, 2020 00:11
This was referenced Sep 1, 2020
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.

Investigate OS errors returned for encryption scenarios on Windows and MacOS
2 participants