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

Update episode 5 and 6 #56

Merged
merged 11 commits into from
Jan 26, 2022
Merged

Conversation

ALuesink
Copy link
Contributor

@ALuesink ALuesink commented Dec 2, 2021

Still in progress

@netlify
Copy link

netlify bot commented Dec 2, 2021

✔️ 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

@ALuesink
Copy link
Contributor Author

ALuesink commented Dec 3, 2021

Episode 5 - Documentation and Citation is migrated to the intermediate tutorial (common-workflow-lab/cwl-intermediate-tutorial#7)

@ALuesink
Copy link
Contributor Author

ALuesink commented Dec 3, 2021

I've added a new episode at the end called More information with links to useful websites

@ALuesink ALuesink marked this pull request as ready for review December 3, 2021 17:00
@mr-c
Copy link
Collaborator

mr-c commented Jan 12, 2022

@gcapes Thanks for all your assistance! Can you review this PR? Since it is bigger I want to finish it before reviewing yours

@gcapes
Copy link
Contributor

gcapes commented Jan 12, 2022

Sure - I'll start on it tomorrow. FWIW my PRs focus on the earlier episodes so could be reviewed independently of this one.

gcapes
gcapes previously requested changes Jan 13, 2022
Copy link
Contributor

@gcapes gcapes left a 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.

_episodes/debug.md Outdated Show resolved Hide resolved
_episodes/debug.md Outdated Show resolved Hide resolved
_episodes/debug.md Outdated Show resolved Hide resolved
_episodes/debug.md Outdated Show resolved Hide resolved
@gcapes
Copy link
Contributor

gcapes commented Jan 21, 2022

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.

@mr-c
Copy link
Collaborator

mr-c commented Jan 21, 2022

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 gh pr checkout 56 using https://cli.github.com/ ?

@gcapes
Copy link
Contributor

gcapes commented Jan 21, 2022

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.

@mr-c
Copy link
Collaborator

mr-c commented Jan 21, 2022

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

@gcapes
Copy link
Contributor

gcapes commented Jan 21, 2022

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.

@gcapes
Copy link
Contributor

gcapes commented Jan 21, 2022

Can someone suggest how to reproduce the tabs instead of spaces error?

@mr-c
Copy link
Collaborator

mr-c commented Jan 21, 2022

@ALuesink do you still have the error.cwl file?

@ALuesink
Copy link
Contributor Author

@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?

@gcapes
Copy link
Contributor

gcapes commented Jan 21, 2022 via email

- Also show the input code which leads to the error
  so learners can reproduce them.
Comment on lines +47 to +50
~~~
cwlVersion: v1.2
class: Workflow

Copy link
Collaborator

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

Copy link
Contributor

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?

@gcapes
Copy link
Contributor

gcapes commented Jan 25, 2022

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.

@mr-c mr-c enabled auto-merge (squash) January 26, 2022 14:35
@mr-c mr-c dismissed gcapes’s stale review January 26, 2022 16:04

Issues are addressed

@mr-c mr-c merged commit ba9dff8 into carpentries-incubator:gh-pages Jan 26, 2022
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