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

fix __repr__ of dataset #196

Merged
merged 3 commits into from
May 26, 2016
Merged

fix __repr__ of dataset #196

merged 3 commits into from
May 26, 2016

Conversation

peendebak
Copy link
Contributor

The .name and .array_id fields of a DataArray can be None. This fixes an error generated when a DataSet contains such a DataArray.

@alexcjohnson @giulioungaretti Is it an idea to gather multiple of these small fixed into a single branch like onelinefixes. This would decrease the number of PRs.

@MerlinSmiles
Copy link
Contributor

Thanks, good catch :)
I'd prefer to have '' instead of 'None' that makes it a bit clearer to me that it is missing.

Where did you run into this?

It just happened to me when I was running the load-old-data notebook while working on the array_id's Basically when a snapshot is missing it wont have that information.

@alexcjohnson
Copy link
Contributor

Is it an idea to gather multiple of these small fixed into a single branch like onelinefixes. This would decrease the number of PRs.

I'd say we've actually been erring on the other side, PRs that are too big - I'm no exception to that - but ideally a PR has a single topic - fixing a single bug, adding a single feature. If that only takes one line, all the better, it'll be super easy to review and approve.

And to that point, good catch, 💃 !
@MerlinSmiles thanks for the linting.

@MerlinSmiles
Copy link
Contributor

@alexcjohnson do you have a preference for none vs empty string as a replacement?

@alexcjohnson
Copy link
Contributor

do you have a preference for none vs empty string as a replacement?

Not really. array_id should probably never be None as long as the array is part of a DataSet (which it clearly is since this is the DataSet repr!) - because it should match the array's key in DataSet.arrays. name I'm not sure we'll even have a use for once we sort out better array_id conventions (ala #197 ) so I see this as a short-term fix anyway.

@MerlinSmiles
Copy link
Contributor

not sure about the ids but names can be empty I think. And also I dont think name should be the same as the ID, is that what you mean.

@peendebak
Copy link
Contributor Author

@MerlinSmiles I found the issue when writing a dataset to disk and then loading it again.
@alexcjohnson I agree that array_ids should not be None. Best thing would be to remove the default initializer in the DataSet constructor, but I'll leave that for another PR.

@peendebak peendebak merged commit db495d3 into master May 26, 2016
@peendebak peendebak deleted the formatter/GNUPlot branch May 26, 2016 21:40
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.

None yet

4 participants