-
Notifications
You must be signed in to change notification settings - Fork 79
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.
This is a really great lesson! I love that each of the stages are fairly short, packing a lot of information into a short space, meaning the reader can get up to speed on robot without knowing a lot, very quickly. This is in keeping with the spirit of existing lessons.
My main feedback at the moment is around basic housekeeping stuff. See my comments inline as well as below:
There are a lot of commits in this PR with really nondescriptive names. If these are merged as-is, it would clutter up the history with a lot of changes that make it hard to track what's being changed where. I can squash this for you but I'd rather you do it so you get credit for the commit(s). Please use git reset <commit id> --soft
to backtrack to one of the earliest commits and then re-commit the changes using more descriptive commit messages. Quality over quantity here. Then, you can force-push your amended branch, and the changes will show up in this PR.
Please remove the following files from the branch:
- lessons/lesson-29/output.xml
- lessons/lesson-29/log.html
- lessons/lesson-29/report.html
- lessons/lesson-29/pycache/substring.cpython-37.pyc
- lessons/lesson-29/pycache/JunosDevice.cpython-37.pyc
Please also delete:
- lessons/lesson-29/stage1/lessondiagram.png
- lessons/lesson-29/stage2/lessondiagram.png
and instead place one at the lesson root. Also update the syringe.yaml file accordingly.
Also, note that CI is failing because no CHANGELOG update. Please update the changelog as indicated in other sections. |
Thanks for the review @Mierdin. We will update the lesson as per your comments. |
f935a1d
to
c2e9adc
Compare
CHANGELOG updated |
All the commits have been squashed. |
@Mierdin We have made all the requested changes. Please review and let us know if we need to make any other modifications. Thanks! |
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.
LGTM. Good work!
Thanks @Mierdin |
@Mierdin @cloudtoad Please review the changes.
cc @saimkhan92