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

Add exercise Circular Buffer #664

Closed
wants to merge 4 commits into from
Closed

Conversation

glennj
Copy link
Contributor

@glennj glennj commented Feb 17, 2024

I had a thought about how to implement this. It's a library like the list-ops exercise.

I'm curious what you all think.

Because it's a library, the tests don't use the bats run command, so assert_success and assert_failure don't apply. You'll see test assertions look like this

    buffer::write buff 'A'      && result=ok || result=fail
    assert_equal "$result" ok

I didn't write any additional instructions. It's a fairly difficult exercise, so users are on their own.

@glennj glennj reopened this Feb 17, 2024
@glennj glennj requested a review from a team February 17, 2024 02:16
@kotp
Copy link
Member

kotp commented Feb 17, 2024

Get it out there and see what falls from the sky‽

@glennj glennj requested a review from IsaacG February 17, 2024 16:53
@glennj
Copy link
Contributor Author

glennj commented Feb 17, 2024

I had a thought just now. What if, instead of

buffer::overwrite bufname value

we had

buffer::write -f bufname value

That would get the student to do option handling inside the function.

That's a more shell-like syntax, but adds complexity.

Worth it?

@kotp
Copy link
Member

kotp commented Feb 18, 2024

Is this the exercise we want to introduce additional complexity in? It is already admittedly somewhat difficult.

Copy link
Member

@IsaacG IsaacG left a comment

Choose a reason for hiding this comment

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

The PR contents look good to me. The idea behind it...

I'm a bit iffy here based on the complexity. Just because you can get bash to do this doesn't mean it's reasonable to do so. I'm leaning more towards passing on exercises that don't fit the language vs jumping through hoops to get things working.

# To use `getopts` in a function, these vars must be local.
local OPTIND OPTARG
while getopts :f opt; do
case $opt in
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
case $opt in
case "$opt" in

@glennj
Copy link
Contributor Author

glennj commented Feb 18, 2024

I'm a bit iffy here based on the complexity. Just because you can get bash to do this doesn't mean it's reasonable to do so. I'm leaning more towards passing on exercises that don't fit the language vs jumping through hoops to get things working.

That is completely reasonable. I'm content to close this.

I'll create an issue so we can discuss foregoing exercises.

@glennj
Copy link
Contributor Author

glennj commented Feb 18, 2024

Although this is do-able in bash, it doesn't mean that it's smart to do it.

@glennj glennj closed this Feb 18, 2024
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.

3 participants