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

feat: Add box1.rs exercise #409

Merged
merged 3 commits into from
May 30, 2020

Conversation

AlexandruGG
Copy link
Contributor

Having completed all of the existing exercises, I noticed there wasn't one with the Box type.

Although this exercise alone might not completely solve #403, I think it's a start 🦀 .

info.toml Outdated

Step 2
Creating an empty list should be fairly straightforward (hint: peek at the assertions).
For a non-empty list keep in mind that wee want to use our Cons "list builder".
Copy link
Contributor

Choose a reason for hiding this comment

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

wee -> we

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good spot, macbook butterfly keyboard 😅 . I'll correct this and put the hints through Grammarly


// Do not change these
assert_eq!(List::Nil, empty_list);
assert_ne!(empty_list, non_empty_list);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a clever way of requiring a non-Nil list variant without giving away the solution!

@danwilhelm
Copy link
Contributor

danwilhelm commented May 26, 2020

This is a good entry-level exercise with good accompanying explanations.

Based on prior Rustlings feedback, the main complaint might be that its solution is inside the Rust book. Personally, I believe this is fine since it gives people an opportunity to play with the ideas in the book.

  • Notice in this screenshot that when the asserts fail, no error is displayed with rustlings watch. This may be confusing to the user, so I suggest placing the asserts into tests.
    image

@AlexandruGG
Copy link
Contributor Author

AlexandruGG commented May 27, 2020

This is a good entry-level exercise with good accompanying explanations.

Based on prior Rustlings feedback, the main complaint might be that its solution is inside the Rust book. Personally, I believe this is fine since it gives people an opportunity to play with the ideas in the book.

  • Notice in this screenshot that when the asserts fail, no error is displayed with rustlings watch. This may be confusing to the user, so I suggest placing the asserts into tests.
    image

Thank you for the feedback!

On the first point: I know the book readily provides the solution, but I thought this could serve as an easy intro to Boxes, and you still get the satisfaction of solving it by reading the book in my view as well :).

On the second point: I hadn't noticed that before because I tested with rustlings run, where the full panic message is displayed. However, I realised I can move my exercise to the top of info.toml to test with rustlings watch 😄 . So I've refactored it now with tests.

@AlexandruGG
Copy link
Contributor Author

AlexandruGG commented May 27, 2020

unrelated question: would it be ok to add .idea to the .gitignore? I think it would be useful for those using IntelliJ with Rust, as the IDE generates this folder

Screenshot 2020-05-27 at 10 17 14

@danwilhelm
Copy link
Contributor

danwilhelm commented May 28, 2020

I tried it out, and it works!

Perhaps someone more involved with the project could weigh in on whether it is okay for the solution to be in the Book? In my opinion, adding this as-is is an improvement to what already exists. Box is often seen in Rust code, yet it was not previously covered by Rustlings.

(A possible variant would be using a linked list rather than a cons list, e.g. at the bottom of https://rust-unofficial.github.io/too-many-lists/first-layout.html.)

PS - @fmoko gave you the thumbs up on .idea, so I'm assuming that's the official go-ahead :D

@shadows-withal
Copy link
Member

PS - @fmoko gave you the thumbs up on .idea, so I'm assuming that's the official go-ahead :D

Yep, go ahead!

@AlexandruGG
Copy link
Contributor Author

PS - @fmoko gave you the thumbs up on .idea, so I'm assuming that's the official go-ahead :D

Yep, go ahead!

Thanks, added now!

What's the next step for this?

@jrvidal
Copy link
Contributor

jrvidal commented May 29, 2020

Will look at this ASAP

Copy link
Contributor

@jrvidal jrvidal left a comment

Choose a reason for hiding this comment

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

LGTM

@shadows-withal shadows-withal merged commit 5f08069 into rust-lang:master May 30, 2020
@AlexandruGG
Copy link
Contributor Author

AlexandruGG commented May 30, 2020

thanks everyone, I had a good time writing this and using rustlings in general. This was my first ever contribution to an open-source project 😁

ppp3 pushed a commit to ppp3/rustlings that referenced this pull request May 23, 2022
dmoore04 pushed a commit to dmoore04/rustlings that referenced this pull request Sep 11, 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.

5 participants