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

ergonomics and added features #21

Merged
merged 52 commits into from Oct 8, 2019
Merged

ergonomics and added features #21

merged 52 commits into from Oct 8, 2019

Conversation

magnusuMET
Copy link
Member

@magnusuMET magnusuMET commented Oct 3, 2019

This PR makes the crate more ergonomic for users, and adds some missing functionality such as unlimited dimensions.

Removed:

  • mutex from netcdf-sys
  • direct access to internal structures

Changed:

  • getters and setters for dimensions/variables/attributes
  • funtions returns a netcdf::Result
  • functions now take Option<&[usize]> for indexes/sizes
  • ndarray is now feature gated
  • tests produces files in a temporary directory

Added:

  • can open from any Path-like item
  • Deref and DerefMutfor the file makes adding and accessing variables/dimensions easier
  • unlimited dimensions
  • read from a memory slice (feature gated due to travis netcdf version)
  • tests

src/variable.rs Show resolved Hide resolved
src/variable.rs Outdated Show resolved Hide resolved
@magnusuMET
Copy link
Member Author

Thank you for the review @bluss! I think the PR is ready to be merged, and published to crates.io

Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
netcdf-sys/Cargo.toml Outdated Show resolved Hide resolved
@bluss
Copy link
Contributor

bluss commented Oct 8, 2019

Nice. I'm just coaching here. Now that it's ready for merge, I'd go back to the PR title and description and edit them to make sure they match the updated content of the PR. For example, the title should be updated. I'd ask for a summary of removed, added and changed features.

@mhiley
Copy link
Collaborator

mhiley commented Oct 8, 2019

I'm good with 0.2.0. Thanks for all the feedback @bluss and awesome work @magnusuMET ! Just confirming one more time since there's been a little more action, are we ready to merge?

@magnusuMET
Copy link
Member Author

We are almost ready, I would like to change the title and description for the PR

@mhiley
Copy link
Collaborator

mhiley commented Oct 8, 2019

Ok sounds good - @magnusuMET you should've received an invite from crates.io to allow you to publish the crate. This way you don't have to wait on me.

@magnusuMET magnusuMET changed the title refactor ergonomics and added features Oct 8, 2019
@magnusuMET
Copy link
Member Author

@mhiley Updated and ready to merge! Still haven't gotten the invite, so you may go ahead and publish.

@mhiley mhiley merged commit d4deb29 into georust:master Oct 8, 2019
@magnusuMET
Copy link
Member Author

Oh, and this also closes #13, #14

@magnusuMET
Copy link
Member Author

And #11 can be closed, along with #16 when the new version is published

@milesgranger
Copy link

Awesome work @magnusuMET and I really appreciate the time taken by @bluss in your detailed review; always learning from your contributions! 👏

@mhiley
Copy link
Collaborator

mhiley commented Oct 8, 2019

published to crates.io as v0.2.0 and tagged repo v0.2.0 🎉 Let me know if there are any problems with the published crate.

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

5 participants