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

Replace use of apptools sweetpickle #48

Merged
merged 3 commits into from
Mar 19, 2021

Conversation

aaronayres35
Copy link
Contributor

Closes #45

This PR replaces the use of apptools sweetpickle with apptools.persistence and pickle from the standard library (as was done in enthought/apptools#199)

I believe the relevant code is covered by tests (e.g.

def test_persistence():
""" Can DataContexts round-trip through the persistence mechanism?
"""
d = DataContext(name='test_context')
d['a'] = 1
d['b'] = 2
f = BytesIO()
d.save(f)
f.seek(0, 0)
d2 = DataContext.load(f)
assert d.name == d2.name
assert set(d2.keys()) == set(['a', 'b'])
assert d2['a'] == d['a']
assert d2['b'] == d['b']
) , and running the test suite locally, tests all still passed.
As a sanity check it would be good to test with an application that depends on codetools DataContext

Copy link
Contributor

@rahulporuri rahulporuri left a comment

Choose a reason for hiding this comment

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

LGTM. I tested this against an internal application and the application is now happy.

Looks like there's a test (test_persistence) which makes me slightly happy.

It'd also be useful if we could store a pickled DataContext in the codebase and run tests against it - to ensure that we're maintaing backwards compatibility in terms of storing/restoring the DataContext objects.

@aaronayres35
Copy link
Contributor Author

aaronayres35 commented Mar 18, 2021

It'd also be useful if we could store a pickled DataContext in the codebase and run tests against it - to ensure that we're maintaing backwards compatibility in terms of storing/restoring the DataContext objects.

Just to make sure I'm on the same page, so you mean we create a data context and save it to some file (this is a one time thing). Then we store that file in like tests/data or something, and in various tests, unpickle the object from that file and use it as the DataContext being tested?

Also should this be done in this PR, or a follow up PR and we push this through as is?

@rahulporuri
Copy link
Contributor

Just to make sure I'm on the same page, so you mean we create a data context and save it to some file (this is a one time thing). Then we store that file in like tests/data or something, and in various tests, unpickle the object from that file and use it as the DataContext being tested?

Yup. That's exactly what i mean.

Also should this be done in this PR, or a follow up PR and we push this through as is?

We can push this through as is - but it'd be good to add that pickle/test sooner rather than later so we can move on and not have codetools in the back of our minds.

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.

Should PersistableMixin fall back to stdlib pickle library if apptools is not installed
2 participants