-
Notifications
You must be signed in to change notification settings - Fork 16
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
Many changes #149
Many changes #149
Conversation
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.
Great code improvements! Some minor comments and suggestions from my side.
json_output="results/${1}_tools.json" | ||
|
||
if [[ $1 =~ "01" ]]; then |
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.
In the long run, it will probably be cleaner to add an extra-repositories
flag and run it ones independently of the matrix CI.
bin/extract_all_tools.sh
Outdated
fi | ||
else | ||
echo "Tool extraction"; |
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.
Is this func to allow CI independent run of the WF ? - then nice
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.
Yes that is the idea. Having the test and other commands in 1 script
bin/get_community_tools.sh
Outdated
|
||
fi; | ||
done | ||
if [ ! -z $1 ] |
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.
So if the there is a variable for this script, only run the filter on microgalaxy ? is this for the test ?
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.
can we make it more implicit, like if [ $1=="test" ]
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.
So if the there is a variable for this script, only run the filter on microgalaxy ? is this for the test ?
Yes it is for the test
--all-tools-json "results/test_tools.json" \ | ||
--planemo-repository-list "test.list" \ | ||
--test | ||
bash bin/extract_all_tools.sh test |
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.
why did u switch back to the bash script, does set -e
work to make it stop on python error ?
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.
To have only one location to modify when we modify the Python script and avoid forgetting
# Usage | ||
|
||
## Prepare environment | ||
# Prepare environment |
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 would add this below Extract all tools outside a GitHub Action
. And any objection to use conda instead of virtualenv ?
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.
And any objection to use conda instead of virtualenv ?
As we could have both virtualenv and conda documented. I usually favor virtualenv when we have Python only project (even if I use it within a conda env 😅) because people might prefer avoid using conda then
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 would add this below Extract all tools outside a GitHub Action.
I get the point. The only thing is it is useful for Training too
…extractor into code_work
Co-authored-by: paulzierep <[email protected]>
Nice, the tests work as well ! |
Co-authored-by: paulzierep <[email protected]>
tools.html
andtutorials.html
- Fix Add tutorial interactive tables #129index.html
embedingtools.html
andtutorials.html