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

Ensure processor has extracted all members when members=None #365

Merged
merged 11 commits into from
Feb 19, 2024

Conversation

jni
Copy link
Contributor

@jni jni commented Jun 17, 2023

Here's an early attempt at fixing #364. Currently, test_unpacking is failing
because of the log tests. I'm not 100% sure what to do about them because the
log becomes pretty complex, so I don't know how to generate the expected log.
Maybe this is the wrong approach — happy to revert the last commit and try
something else if you have suggestions! 🙏

Commits

Relevant issues/PRs:

Fixes #364

@welcome
Copy link

welcome bot commented Jun 17, 2023

💖 Thank you for opening your first pull request in this repository! 💖

A few things to keep in mind:

No matter what, we are really grateful that you put in the effort to do this!

@jni
Copy link
Contributor Author

jni commented Jun 29, 2023

👋

@leouieda
Copy link
Member

Hi @jni sorry for the very very long delay on this. We'll try to put out the fire on #373 and then get on this.

@leouieda
Copy link
Member

@jni I think the problem is that you're changing the value of self.members. There is a check in _extract_file for self.members is None to decide how to extract and that check is now always false. I think the fix would be to set a local members variable in __call__ that either is self._all_members(fname) or self.members.

@jni
Copy link
Contributor Author

jni commented Oct 30, 2023

Thanks @leouieda! I'm sure I have older neglected PRs as a maintainer. 😂 And your suggestion seems to have done the trick! 🚀

Copy link
Member

@leouieda leouieda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jni small comment on the test and I think Black and pylint are complaining 😬

Thanks for this!

pooch/tests/test_processors.py Outdated Show resolved Hide resolved
@jni
Copy link
Contributor Author

jni commented Dec 6, 2023

@leouieda icymi I made the suggested changes! 👆

@leouieda
Copy link
Member

@jni sorry for the delay on this. I'll get these documentation issues fixed in main and then merge this asap. Thanks for your help!

@jni
Copy link
Contributor Author

jni commented Feb 17, 2024

@leouieda don't worry, I am very comfortable with long-lived PRs. 😂 I appreciate your efforts! 🙏 Please let me know if I can do anything else.

This way we don't have to test against using it since it can't be
instantiated directly. Make _all_members and _extract_file abstract
methods.
Make it required to be implemented by child classes so we don't have to
check if it's None.
@leouieda leouieda merged commit ec7f3ee into fatiando:main Feb 19, 2024
18 checks passed
Copy link

welcome bot commented Feb 19, 2024

🎉 Congrats on merging your first pull request and welcome to the team! 🎉

If you would like to be added as a author on the Zenodo archive of the next release, add your full name, affiliation, and ORCID (optional) to the AUTHORS.md file of this repository. Feel free to do this in a new pull request if needed.

We hope that this was a good experience for you. Let us know if there is any way that the contributing process could be improved.

@leouieda
Copy link
Member

Alright! I made some slight modifications to make ExtratorProcessor an abstract base class. That got rid of the need to check if methods are implemented and made the tests and checks pass with our new pylint configuration.

Thanks for the contribution @jni! ❤️

@jni jni deleted the members-no-members branch February 20, 2024 00:07
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.

If Unzip/Untar fails because of a non-existent member, calling without members won't work
2 participants