search

exercism / r

Exercism exercises in R.
https://exercism.org/tracks/r
MIT License
27 stars 38 forks source link

[New practice exercise] Robot simulator #326

Closed Nerwosolek closed 7 months ago

Nerwosolek commented 7 months ago

As we talked I proposed the approach with S3 class called "robot" and one generic method and one class method. Becuase it is hard to force the implementation in this case I tried to suggest the implementation via:

In example.R I proposed how I imagine S3 implementation for this exercise. I wanted it to be simple, cause this exercise does not justify full blown S3 implementation as there is only one class we can think of. WDYT? Can I improve it somehow?

github-actions[bot] commented 7 months ago

Hello. Thanks for opening a PR on Exercism 🙂

We ask that all changes to Exercism are discussed on our Community Forum before being opened on GitHub. To enforce this, we automatically close all PRs that are submitted. That doesn't mean your PR is rejected but that we want the initial discussion about it to happen on our forum where a wide range of key contributors across the Exercism ecosystem can weigh in.

You can use this link to copy this into a new topic on the forum. If we decide the PR is appropriate, we'll reopen it and continue with it, so please don't delete your local branch.

If you're interested in learning more about this auto-responder, please read this blog post.

Note: If this PR has been pre-approved, please link back to this PR on the forum thread and a maintainer or staff member will reopen it.

colinleach commented 7 months ago

I don't know why the linter is complaining about the stub file, but not the example.R. We'll need @jonmcalder to comment on that, and any other linter issues.

I'll provisionally approve this now, but merging will need to wait for Jon's comments.

Nerwosolek commented 7 months ago

Approved provisionally, but linter issues need input from another reviewer.

I addressed some of the linter complaints but there are some inner errors of linter itself. Look in the forum. I could take a look at this in the other PR.

Nerwosolek commented 7 months ago

I don't know why the linter is complaining about the stub file, but not the example.R.

I suspect linter can check if move.robot function belongs to the class robot of S3 system, maybe via methods() function.

Nerwosolek commented 7 months ago

@colinleach Could you look again? I solved the problem but the fact I had to use # nolint and it will be visible to student does not satisfy me.

jonmcalder commented 7 months ago

Thanks this looks good! And thanks also for addressing the outdated linter setup.

I'm not too fussed about adding in the #nolint comment in the stub to avoid the linter complaining - I was going to suggest that at least as a stop gap solution for now. I did also consider it might confuse students slightly or be off-putting but even if it's not immediately clear a quick Google should make it obvious why it's there and it doesn't necessarily need to stay as part of their solution.

I just have the one gripe regarding types - feels like we should probably be a little stricter and expect integers, otherwise if we're ok with numerics then the example constructor shouldn't convert the type when creating the object.

jonmcalder commented 7 months ago

On 2nd thought - I just realised I didn't actually address your initial comments / question at the start of the PR regarding implementation approach.

Now that I think of it, I didn't necessarily expect move to be implemented as a generic - I was simply thinking that there should (at least) be a robot class. I would be ok with move simply being a function that takes a robot instance plus instructions and modifies and returns a (new) robot with updated coordinates and direction.

Obviously the way you've laid out the stub should prompt students to implement a move generic but since the tests don't actually require or test for that it could be done either way which I think is ok so (pending the issue regarding numeric vs integer types) I'm happy with this implementation as is if you and @colinleach are.

Nerwosolek commented 7 months ago

should prompt students to implement a move generic but since the tests don't actually require or test for that it could be done > either way which I think is ok so (pending the issue regarding numeric vs integer types) I'm happy with this implementation as is > if you and @colinleach are.

I'm OK with that of course :). Maybe it's a point for discussion during mentoring session about generics and dispatching. I just wanted to introduce anything more than just a class (as that was already in triangle exercise). Maybe later there will be exercises which give opportunity to build even more sophisticated S3 system based on already acquired knowledge.

jonmcalder commented 7 months ago

Good work and thanks again!