-
Notifications
You must be signed in to change notification settings - Fork 51
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
Add fmt and lint checks #118
Conversation
5732ba9
to
794d24d
Compare
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.
Thanks! Getting closer, just some minor nitpicks.
@bernardopericacho After this PR, do you think we’re getting close to a v1 release or are there other major changes you had in mind? |
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.
I have to rebase the changes from master and apply this again. Hopefully I can do it this weekend
I was thinking about pinging you (somehow) to talk about a couple of things:
|
794d24d
to
caaaba1
Compare
Sorry for the delay! This fell off my radar.
I personally don't think the API is too confusing right now as is. Plus, I think the examples are verbose & helpful enough that it resolves any potential confusion. I think reading/writing directly to/from files is the majority use-case for this library. Could be wrong here. It saves a few steps of the user having to open the file handle. Also, it allows us to read in detached-header NRRD files when reading by opening the header file for them.
That sounds good to me. I'm a big fan of type hinting.
Yep, that's correct. IIRC, it was necessary so ReadTheDocs would know to download that. I should be able to do this a different way now. |
I did this in #120, so we're good to go there. |
Proposed changes