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

Ultima.flow annotations.fix #8442

Merged

Conversation

dror27
Copy link
Contributor

@dror27 dror27 commented Jul 31, 2023

minor changes and fixes to Flow Annotations.

@ilyasoifer
Copy link
Collaborator

@dror27 - can you remind me what we decided to do with this fix? I kind of remember that we decided to not change the definition... But I may be confused

@dror27
Copy link
Contributor Author

dror27 commented Nov 9, 2023

@dror27 - can you remind me what we decided to do with this fix? I kind of remember that we decided to not change the definition... But I may be confused

@ilyasoifer I believe that this has to "simply" be merged.

@ilyasoifer
Copy link
Collaborator

Is it possible to add an integration test to this? Since the change did not fail any test, it seems that the integrationtest is missing

@dror27
Copy link
Contributor Author

dror27 commented Nov 11, 2023

Is it possible to add an integration test to this? Since the change did not fail any test, it seems that the integrationtest is missing

Unit test fixed to include checking of variant type (which it did not include before - hence the non-failure). Test data adjusted accordingly.

@ilyasoifer
Copy link
Collaborator

@meganshand - this is the first PR in the series. PTAL, thanks!

@meganshand meganshand merged commit 683eaa8 into broadinstitute:master Nov 13, 2023
20 checks passed
rickymagner pushed a commit that referenced this pull request Nov 28, 2023
* hmer ondel must have mon length

* Revert "hmer ondel must have mon length"

This reverts commit 7852871.

* remove superfluous variant type condition

* fix error message to actually reflect missing argument

* fixed unittest to include variant type

* Remove conflict
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.

None yet

3 participants