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

App-SpaM #4

Merged
merged 17 commits into from
Nov 20, 2020
Merged

App-SpaM #4

merged 17 commits into from
Nov 20, 2020

Conversation

matthiasblanke
Copy link
Contributor

Hi everybody,
finally we have cleaned up App-SpaM and added it to bioconda. Hence, we would also appreciate to have it added to PEWO. AFAIK I followed all developer instructions, if you find any errors I should fix just let me know. I also had to make some small adjustments in the PEWO_java submodule but couldn't figure out how to include them here - should I just do another pull request at the other repo?

@blinard-BIOINFO
Copy link
Member

Dear Matthias,
Thank you for this ! Really appreciated !
We will carefully review your code and merge it afterwards.

@blinard-BIOINFO
Copy link
Member

blinard-BIOINFO commented Nov 13, 2020

@matthiasblanke Concerning the java side, yes please, do a pull request to this other repo.
I'm just surprisd you had to do something there, it's intended to be independant.
What was the nature of the change ?

@matthiasblanke
Copy link
Contributor Author

Thanks for the fast response. I did another PR at the PEWO_java repo with my changes - maybe they are unnecessary or there is a better way to do it though... If you need any help or more info please let me know.

@blinard-BIOINFO
Copy link
Member

Oh, I see.
Indeed, I will have to change these few lines that were initially for debugging purposes...
Thank you for pointing it !

@nromashchenko
Copy link
Member

Hello,
Good job! I'd just ask one more thing... Our Travis CI runs two pipelines travis/tests/1_..., travis/tests/2_... on every push, making sure it's possible to build and run those toy examples in the isolated environment. In your request you did not change config files of those, and App-SpaM is not tested with CI. "All checks have passed" does not tell anything about App-SpaM yet.
Could you please add App-SpaM there as well? Do not forget to include App-SpaM in test_soft.
Thanks

@matthiasblanke
Copy link
Contributor Author

Ah, I didn't notice those.. added it to the two config files; looks like it worked?

@blinard-BIOINFO
Copy link
Member

Our Travis CI runs two pipelines travis/tests/1_..., travis/tests/2_... on every push, making sure it's possible to build and run those toy examples in the isolated environment. In your request you did not change config files of those, and App-SpaM is not tested with CI.

Good point, we need to add this in the developer documentation.
I'm adding an issue for this.

Change __author__ to Mathias Blanke
@blinard-BIOINFO blinard-BIOINFO merged commit 677b052 into phylo42:master Nov 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants