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

Add fmt and lint checks #118

Merged
merged 3 commits into from
Jul 3, 2022
Merged

Conversation

bernardopericacho
Copy link
Contributor

Proposed changes

  • Add pre-commit hooks to the repository to lint, format and upgrade the code.
  • Add Github action and instructions to run and enforce the pre-commit hooks.

@addisonElliott addisonElliott self-assigned this Jun 14, 2022
Copy link
Collaborator

@addisonElliott addisonElliott left a 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.

nrrd/tests/test_parsing.py Show resolved Hide resolved
nrrd/tests/test_formatting.py Show resolved Hide resolved
nrrd/reader.py Outdated Show resolved Hide resolved
nrrd/reader.py Outdated Show resolved Hide resolved
@addisonElliott
Copy link
Collaborator

@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?

Copy link
Contributor Author

@bernardopericacho bernardopericacho left a 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

nrrd/reader.py Outdated Show resolved Hide resolved
nrrd/reader.py Outdated Show resolved Hide resolved
nrrd/tests/test_parsing.py Show resolved Hide resolved
@bernardopericacho
Copy link
Contributor Author

@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?

I was thinking about pinging you (somehow) to talk about a couple of things:

  • one is maybe we should just support writing and reading from memory and that will make the code much cleaner and the parameters and usage more clear. We can write examples similar to this to demonstrate how you can read from or write to a file. https://github.com/mhe/pynrrd/blob/master/nrrd/tests/test_writing.py#L374
  • the other one was typing hints. What are your thoughts about it?
  • Also, looks like numpydoc dependency is only used for generating the documentation? (please correct me if I am wrong) it is needed as a package dependency?

@addisonElliott addisonElliott merged commit f7e8e5a into mhe:master Jul 3, 2022
@addisonElliott
Copy link
Collaborator

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.

  • the other one was typing hints. What are your thoughts about it?

That sounds good to me. I'm a big fan of type hinting.

  • Also, looks like numpydoc dependency is only used for generating the documentation? (please correct me if I am wrong) it is needed as a package dependency?

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.

@addisonElliott
Copy link
Collaborator

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.

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.

2 participants