Skip to content

bug(fcsfile): fix recursion when file has no spill string #145

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

Merged
merged 1 commit into from
Mar 8, 2022

Conversation

gegnew
Copy link
Contributor

@gegnew gegnew commented Feb 15, 2022

This implementation will change with #136 and the PRs that follow

@gegnew gegnew force-pushed the ge/hotfix-spillstring branch from b91ee61 to 1f8ac87 Compare February 15, 2022 17:23
@gegnew gegnew changed the title ge/hotfix spillstring bug(fcsfile): fix recursion when file has no spill string Feb 15, 2022
@gegnew gegnew requested review from zbjornson and gingo00 February 15, 2022 17:24
Copy link

@gingo00 gingo00 left a comment

Choose a reason for hiding this comment

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

I confirm that it does fix the recursive call issue.

Possibly, like Zach suggested in slack, the get_file_internal_compensation method could verify the hasFileInternalComp attribute and throw a more meaningful error (or return None) if hasFileInternalComp is false. Otherwise, the user get a more cryptic ValueError: invalid literal for int() with base 10: '' from the Compensation.from_spill_string method.

@gegnew gegnew force-pushed the ge/hotfix-spillstring branch from 1f8ac87 to 52c354f Compare February 16, 2022 09:13
@gegnew
Copy link
Contributor Author

gegnew commented Feb 16, 2022

☝️ implemented

@gegnew
Copy link
Contributor Author

gegnew commented Feb 16, 2022

This also accounts for the edge case (which I think only happens in my tests) where hasFileInternalComp = True but there is no spillString

@gegnew gegnew force-pushed the ge/hotfix-spillstring branch 4 times, most recently from c70c92a to cdd49ee Compare February 17, 2022 16:33
@gegnew gegnew requested review from zbjornson and gingo00 February 28, 2022 12:11
@gegnew gegnew requested a review from zbjornson March 1, 2022 16:51
@zbjornson
Copy link
Member

@gegnew you requested review but I don't see a new push.

@gegnew gegnew force-pushed the ge/hotfix-spillstring branch from cdd49ee to 5b8edb5 Compare March 1, 2022 20:30
@gegnew
Copy link
Contributor Author

gegnew commented Mar 1, 2022

🤦 sorry, forgot to push it after fixing merge conflicts

@gegnew gegnew force-pushed the ge/hotfix-spillstring branch from 5b8edb5 to d766bcd Compare March 7, 2022 05:52
Copy link
Member

@zbjornson zbjornson left a comment

Choose a reason for hiding this comment

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

Great.

@zbjornson zbjornson merged commit da2c7dc into master Mar 8, 2022
@zbjornson zbjornson deleted the ge/hotfix-spillstring branch March 8, 2022 01:48
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