-
Notifications
You must be signed in to change notification settings - Fork 113
json convenience functions #674
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
Conversation
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.
Generally lgtm, except one question about the input and the need for some unit tests (besides the doctests)
| if hasattr(fp, 'write'): | ||
| return json.dump(data, fp, cls=DataEncoder) | ||
| with open(fp, 'w') as fp: |
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 the plan is to fix up all of the file readers so that they take file-like objects and paths, I think these lines of code will be used quite a few times. Maybe there should be a utility function which does this. Some sort of context manager like
class open_file(object):
def __init__(self, fp, *args, **kwargs):
if hasattr(fp, 'write'):
self.f = fp
else:
self.f = open(fp, *args, **kwargs)
def __enter__(self):
return self.f
def __close__(self):
self.f.close()
# but maybe you don't always want to close file-like objects...
And I suppose this could be generalized to handle reading as well.
|
@gonzalocasas @Licini @beverlylytle tests pass now. can we merge this? or do we need more? |
What type of change is this?
Checklist
Put an
xin the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.CHANGELOG.mdfile in theUnreleasedsection under the most fitting heading (e.g.Added,Changed,Removed).invoke test).invoke lint).compas.datastructures.Mesh.