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

Improves error handling when "shear" but no metadata passed #525

Merged
merged 5 commits into from
Jun 10, 2021

Conversation

ChrisKeefe
Copy link
Contributor

When tree_plot is run with shear-to-feature-metadata, but no metadata is passed, the following error is produced.
image

This PR explicitly handles that case, improving the error message:
image

@ChrisKeefe
Copy link
Contributor Author

ChrisKeefe commented Jun 9, 2021

Fixed the linter error I introduced, but left the other two alone to keep the diff minimal. I can fix these other two here if you like.
image

@fedarko fedarko self-requested a review June 10, 2021 07:31
Copy link
Collaborator

@fedarko fedarko left a comment

Choose a reason for hiding this comment

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

Thank you @ChrisKeefe! I have one small comment -- I think we could catch this error a bit earlier, but in any case this is really useful to have fixed.

I am not sure about those linting errors -- from what I can tell, your code passes all of the linting stuff on my system (running make stylecheck in the root of the repository), and it seems now to be passing in our CI. Maybe our flake8 versions are slightly different? (Looks like flake8 v3.5.0 and v3.9.2 both accept this PR's code without a problem now.) Not a big deal, though!

EDIT: Actually omg I think I figured out why you got linting errors and I didn't -- so what's happening is the difference is our shell, and/or the make version we have installed. On my system (Ubuntu, using the fish shell), flake8 empress/*.py only expands to catch the python files one level down -- so it doesn't catch the python files in the empress/scripts/ directory. However, I think your system is being smart, and actually is expanding that glob to include all of the stuff in empress/scripts/, which has apparently never been linted until now :O

... So, if you wouldn't mind adding empress/scripts/*.py to the stylecheck Makefile directive, and fixing those two linting errors (it's just adding a few blank lines), that would solve that problem. Thank you for catching that!!! 🌳 🌳 🌳

empress/core.py Outdated Show resolved Hide resolved
@kwcantrell
Copy link
Collaborator

Thanks @ChrisKeefe for catching/fixing the shear-to-feature-metadata bug as well as discovering/fixing the lint issue(s)! This a two-for-one PR! 💯🥇 💯

Since you are a first time contributor, I will go ahead an approve the workflows so that our CI can run and after that I this should be good to merge into master.

@kwcantrell kwcantrell merged commit 4f1ed07 into biocore:master Jun 10, 2021
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.

3 participants