Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: Install subworkflows with modules from different remotes #3083
base: dev
Are you sure you want to change the base?
feat: Install subworkflows with modules from different remotes #3083
Changes from 66 commits
9825d89
568363a
534da92
286ab86
423517e
6816cc1
af816fd
d0e6031
1737c88
f7aaea2
d1c490d
6fa5cf4
d903282
7f095fa
012e9d6
d77fccb
faa7faf
0f08bbe
6d052de
c6ce67f
792f68b
7d23f15
6f9a60e
fca6f27
092bf91
208d796
71c43be
8bcf737
efde7b7
8e25587
4947913
4888d32
aa265d8
ab0f398
33c36cf
9356cd3
a764c76
869d816
c3ac1b8
4eb95e6
e9bf238
c8b8a83
eba8391
e3f7b3f
ddb3dd0
634ff8f
c72b94a
bb31d78
7f4cce8
e342678
50216c4
0e83d9a
f8c8251
313853e
e8e4da0
f783d58
eb59308
be4d78f
3c493b9
b13a406
e96341e
2b9a573
e55c74d
4b9fea5
ea7d1ca
a40d5d9
2491207
a870787
7ad4a8c
f3259bd
1a8c3be
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the
isinstance
check is needed (and indeed even if it isn't - because this is always a dict now) then the type hint in the method signature is incorrect.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I could see, this is the single
isinstance
check that was actually needed. There are some places in the codebase where this function gets called with a string input, when removing the isinstance check I noticed quite a few test failures. So, to aim for minimal impact and changes to the codebase and current implementations, I've opted to change the argument type hint, as you suggested.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can use the nf-core gitlab account for this test to avoid using external repos. I can give you access if you think it's a good idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure on this. I don't recommend using two remotes with the same org path as this can create major foot-guns (tools will just overwrite the ones in place and there can be weirdness in the modules json).
Might be OK for testing if you are careful but in general it is not something people should try to do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do agree that using a different org_path is better for testing purposes. Although we don't have to keep it in my personal remote either, I was planning to make that repo read-only as soon as this feature is done, to ensure future stability for the test.
I don't know if it's simple to setup on your side, but maybe something like "nf_core_testing" could be a good alternative org_path? It's different enough but still under nf-core, instead of a personal user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, I will have a loot at the options we have to create another nf-core owned organisation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/nf-core-test/modules I have added you both to the new organisation