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

stack/intro: Lab 8 - Stiva #22

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

george-cosma
Copy link

@george-cosma george-cosma commented Apr 24, 2024

Prerequisite Checklist

  • Read the contribution guidelines regarding submitting new changes to the project;
  • Tested your changes against relevant architectures and platforms;
  • Updated relevant documentation (if needed).

Description of changes

Initial draft done, can be reviewed

@george-cosma george-cosma marked this pull request as ready for review April 24, 2024 20:17
@george-cosma george-cosma changed the title stack/intro: Lab 8 WIP stack/intro: Lab 8 - Stiva Apr 24, 2024
@george-cosma
Copy link
Author

Am sa dau fix la erori cam vineri-ish, sau pe weekend

@github-actions github-actions bot added area/tasks Update to tasks topic/stack Related to the "Stack" chapter kind/new New content / item labels Apr 25, 2024
Copy link

@teodutu teodutu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The content is mostly fine. Most of my inline suggestions apply to all code files. In addition

  1. Fix the github action test failures [1]. For the spellcheck false positives, you'll have to add the missing words to these wordlists here [2] as a separate PR.
  2. Squash your commits to a single one named something like stack/stack: Add lab exercises, guides and reading + a description and signature.

[1] https://github.com/cs-pub-ro/hardware-software-interface/pull/22/checks
[2] https://github.com/cs-pub-ro/hardware-software-interface/actions/runs/8829121620/job/24239469518?pr=22

chapters/stack/intro/drills/tasks/gcd/solution/.gitignore Outdated Show resolved Hide resolved
chapters/stack/intro/drills/tasks/gcd/README.md Outdated Show resolved Hide resolved
chapters/stack/intro/drills/tasks/gcd/solution/Makefile Outdated Show resolved Hide resolved
chapters/stack/intro/drills/tasks/gcd/solution/gcd.asm Outdated Show resolved Hide resolved
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remake this image as .svg and rename it to process-address-space.svg. We only use snake_case for source code file names.

chapters/stack/intro/media/the-stack-growth.drawio.svg Outdated Show resolved Hide resolved
chapters/stack/intro/media/the-stack.drawio.svg Outdated Show resolved Hide resolved
chapters/stack/intro/reading/README.md Outdated Show resolved Hide resolved
@george-cosma george-cosma added needs-rendering The PR makes changes to the website that need to be rendered and removed needs-rendering The PR makes changes to the website that need to be rendered labels May 8, 2024
Copy link

github-actions bot commented May 8, 2024

@george-cosma george-cosma force-pushed the lab8-stack-2 branch 7 times, most recently from 6b2fb61 to 236c61e Compare June 25, 2024 18:23
@george-cosma george-cosma added the needs-rendering The PR makes changes to the website that need to be rendered label Jun 25, 2024
@george-cosma george-cosma added needs-rendering The PR makes changes to the website that need to be rendered and removed needs-rendering The PR makes changes to the website that need to be rendered labels Jun 25, 2024
@george-cosma george-cosma added needs-rendering The PR makes changes to the website that need to be rendered and removed needs-rendering The PR makes changes to the website that need to be rendered labels Jun 25, 2024
Copy link

@george-cosma
Copy link
Author

Don't know how to fix spellcheck. Otherwise done 👍

Copy link

@teodutu teodutu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made inline comments with what needs changing. To fix spellcheck, add the false positives to these wordlists [1] and keep them sorted and avoid duplicates using sort -u -o <file-name> <file-name>.

[1] https://github.com/open-education-hub/actions/tree/main/spellcheck/config

chapters/stack/intro/drills/tasks/gcd/README.md Outdated Show resolved Hide resolved
chapters/stack/intro/drills/tasks/gcd/support/gcd.asm Outdated Show resolved Hide resolved
chapters/stack/intro/drills/tasks/reverse-array/README.md Outdated Show resolved Hide resolved
chapters/stack/intro/reading/README.md Outdated Show resolved Hide resolved
chapters/stack/intro/reading/README.md Outdated Show resolved Hide resolved
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Split this file into the following files and group the exercises and demos around them (where possible) so they form subchapters:

  • introduction.md (intro + stack data structure)
  • stack-operations.md
  • stack-addressing.md

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The content is quite short, I think splitting up the concepts will just end up with more confused students. Teach them what they need to know, then let them practice. Splitting it off so atomically is just not worth it imho

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok then remove the "Introduction" subchapter in config.yaml and make the links in the tasks refer to specific sections in the reading material (using #, such as .../README.md#stack-operations).

config.yaml Show resolved Hide resolved
chapters/stack/intro/media/the-stack-growth.svg Outdated Show resolved Hide resolved
Cool description here

Signed-off-by: George Cosma <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/tasks Update to tasks kind/new New content / item needs-rendering The PR makes changes to the website that need to be rendered topic/stack Related to the "Stack" chapter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants