-
-
Notifications
You must be signed in to change notification settings - Fork 20
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
Update episode 5 and 6 #56
Conversation
✔️ Add Deploy Preview notifications as pull request comments when Deploy Preview succeeds 🔨 Explore the source changes: 3c77e8a 🔍 Inspect the deploy log: https://app.netlify.com/sites/thirsty-hoover-1e0704/deploys/61f170f2542d590007ee9ea8 😎 Browse the preview: https://deploy-preview-56--thirsty-hoover-1e0704.netlify.app |
Episode 5 - Documentation and Citation is migrated to the intermediate tutorial (common-workflow-lab/cwl-intermediate-tutorial#7) |
I've added a new episode at the end called More information with links to useful websites |
@gcapes Thanks for all your assistance! Can you review this PR? Since it is bigger I want to finish it before reviewing yours |
Sure - I'll start on it tomorrow. FWIW my PRs focus on the earlier episodes so could be reviewed independently of this one. |
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.
This looks like a good start!
I've made a few specific wording and typo comments, but in addition would it be useful to have a section on array vs single-item mismatch? That was in the original outline.
I would also suggest runnable examples here. Perhaps as exercises, and/or the input files for the error outputs shown.
Finally, I think that code output boxes would be preferable to screenshots. Certainly for maintainability - if someone else wants to add/modify the example it is likely their screenshots will look different.
I've now fixed all the suggestions, except I can't reproduce the error from "Using tabs instead of spaces". I also seem unable to push to this PR's branch, so I'll make a new PR to replace this one. |
Did you try |
No, I've not used the github cli, but that command would only check out the branch (wouldn't it?), which I've already done. I've locally rebased onto gh-pages, and made the fixes. |
It sets up the remote to push as well. Anyhow, next time |
Look like I just figured it out :) I'd renamed my local branch, and it looks like that has to match the remote branch name. |
Can someone suggest how to reproduce the tabs instead of spaces error? |
@ALuesink do you still have the |
Yes, I have it. However, I now see it has no tabs so it would not create the error. I've tried a file with tabs instead of spaces, but I don't get the |
Thanks for checking. I get the same error I when trying to reproduce an example with tabs. I'll add an example to this episode next week.
Thanks
Gerard
--
Gerard Capes
Research Applications, IT Services, University Of Manchester
Hours: Tues - Fri, 08:00 - 16:00
…________________________________
From: ALuesink ***@***.***>
Sent: 21 January 2022 16:47
To: carpentries-incubator/cwl-novice-tutorial ***@***.***>
Cc: Gerard Capes ***@***.***>; Mention ***@***.***>
Subject: Re: [carpentries-incubator/cwl-novice-tutorial] Update episode 5 and 6 (PR #56)
@ALuesink<https://github.com/ALuesink> do you still have the error.cwl file?
Yes, I have it. However, I now see it has no tabs so it would not create the error. I've tried a file with tabs instead of spaces, but I don't get the found character \t error. This time I get 'NoneType' object has no attribute ....
Is it possible the error has changed in the meantime?
—
Reply to this email directly, view it on GitHub<#56 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AC66UPXFY2E73J7X6VLR253UXGE2VANCNFSM5JHVK57A>.
Triage notifications on the go with GitHub Mobile for iOS<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
- Also show the input code which leads to the error so learners can reproduce them.
~~~ | ||
cwlVersion: v1.2 | ||
class: Workflow | ||
|
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.
Maybe a shorter example? This is currently longer than a screenful with not much advantage for that
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.
There's a tradeoff between using the example from the rest of the lesson that learners will be familiar with vs using a 'hello world' example which would be shorter but unrelated. What do you think?
This is now ready for review/merge. The 'wiring' error I haven't done, because I can't actually run the workflow in this lesson. It crashes my laptop part way through. I would think these would work well as exercises rather than examples, but have followed the pattern in the original submission for now. |
Still in progress