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

adding sort shortest to longest and vice versa #41

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

nuclearkatie
Copy link
Member

@nuclearkatie nuclearkatie commented Jan 14, 2022

  • added sort shortest to longest
  • added check for user error in submitting pathways list that would cause many sort/filter functions to fail, raises exception
  • 100% coverage on pathway_analysis (coveralls)
  • linting on pathway_analysis and tests

@gonuke gonuke self-requested a review January 14, 2022 19:13
Copy link
Member

@gonuke gonuke left a comment

Choose a reason for hiding this comment

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

This is reasonably straightforward - but one philosophical question: Could you instead check for invalid pathways upon creation of a list of pathways? I guess that might not make sense because you can't control where a list of pathways comes from, but you could check for invalid pathways in the lists you do generate. Or do those still represent useful information in some instances?

{(5.3)}])
def test_check_for_invalid_pathways_type_error(pathways):
with pytest.raises(TypeError) as excinfo:
obj = pa.sort_by_longest(pathways)
Copy link
Member

Choose a reason for hiding this comment

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

why not just call check_for_invalid_pathways here?

Comment on lines +601 to +602
obs = pa.check_for_invalid_pathways(pathways)
assert obs is None
Copy link
Member

Choose a reason for hiding this comment

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

Does it make more sense to some how test that there is no exception?

@gonuke
Copy link
Member

gonuke commented Aug 26, 2022

Not sure if this is a priority @nuclearkatie , but it would be nice to clear this up.

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