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 game-of-life exercise #135

Closed
wants to merge 2 commits into from
Closed

Add game-of-life exercise #135

wants to merge 2 commits into from

Conversation

atk
Copy link

@atk atk commented Nov 27, 2024

No description provided.

Copy link

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.

@github-actions github-actions bot closed this Nov 27, 2024
Copy link

Hello 👋 Thanks for your PR.

This repo does not currently have dedicated maintainers. Our cross-track maintainers team will attempt to review and merge your PR, but it will likely take longer for your PR to be reviewed.

If you enjoy contributing to Exercism and have a track-record of doing so successfully, you might like to become an Exercism maintainer for this track.

Please feel free to ask any questions, or chat to us about anything to do with this PR or the reviewing process on the Exercism forum.

(cc @exercism/cross-track-maintainers)

@keiravillekode
Copy link
Contributor

Try running bin/fetch-configlet ; bin/configlet lint

.../exercises/practice contains a directory named game-of-life, which is not a slug in the array of practice exercises. Please add the exercise to that array. If the exercise is not ready to be shown on the website, please set its status value to "wip":

In the root config.json we can add

      {
        "slug": "game-of-life",
        "name": "Game Of Life",
        "uuid": "d6f05d17-49b9-42de-b2fa-d2cc7c5ca11f",
        "practices": [],
        "prerequisites": [],
        "difficulty": 7
      },

above minesweeper.

This is a new UUID, we can generate them using bin/fetch-configlet ; bin/configlet uuid

game-of-life can have difficulty 7 for WASM and MIPS, difficulty 8 for x86 and arm64.
minesweeper can have difficulty 8 for WASM and MIPS, difficulty 7 for x86 and arm64.
This way, anyone seeking Difficult trophies will encounter a variety of exercises.

@keiravillekode
Copy link
Contributor

Again, please run ./bin/fetch-configlet ; ./bin/configlet lint

The root config.json has

  "files": {
    "solution": [
      "%{kebab_slug}.wat"
    ],
    "test": [
      "%{kebab_slug}.spec.js"
    ],
    "example": [
      ".meta/proof.ci.wat"
    ]
  },

so the example solution in the .meta directory should be called proof.ci.wat

@keiravillekode
Copy link
Contributor

I am trying to maintain some consistency across the Web Assembly tracks, by having exercises that write to memory return an (offset, length) pair.

I am also trying to maintain some consistency across assembly tracks, with the intention of Game Of Life using an array of words instead of a string, and also with rowCount being passed before columnCount.

@atk
Copy link
Author

atk commented Dec 9, 2024

Switching the rows and columns argument would be simple, so I don't really understand why this should be a reason for closing this PR.

@keiravillekode
Copy link
Contributor

  • I would like this exercise to represent each row as a 32 bit word, instead of using strings. Otherwise solution code can be very similar to minesweeper solution code. I would like bit manipulation to be a feature of the assembly tracks, and this is an ideal exercise.
  • I would like blocks of memory passed in and out to be described using (offset, length), even when the length can be deduced. Minesweeper and reverse-string and rotational-cipher are examples.
  • count arguments for this track should use the naming convention rowCount columnCount instead of columns rows
  • Local variables would ideally be defined one per line.

When I re-opened the PR, I wasn't expecting to be "overruled" on such matters.

None of the submissions have passed CI (continuous integration) checks. After running a local git commit, you could use ./bin/test to test the example solution. This is mentioned under Testing in the track ReadMe.

@atk
Copy link
Author

atk commented Dec 10, 2024

I would like this exercise to represent each row as a 32 bit word

This would limit us to 32 columns, which is an unrealistic requirement for an actual game-of-life engine, as I have already argued in the forum (you failed to address this argument).

Otherwise solution code can be very similar to minesweeper solution code. I would like bit manipulation to be a feature of the assembly tracks, and this is an ideal exercise.

I really don't see why this would be a bad thing. It would also make much more sense to use Allergies as an example for bit manipulation IMO than to cram that pattern into a task just for the sake of having it.

I would like blocks of memory passed in and out to be described using (offset, length), even when the length can be deduced. Minesweeper and reverse-string and rotational-cipher are examples.

First, you argue against exercises to be too similar and now you argue for consistency. I think consistency is fine when there are no good reasons to deviate from it. I have already explained the reason when opening the PR.

When I re-opened the PR, I wasn't expecting to be "overruled" on such matters.

I made all the changes you requested that I found reasonable and presented arguments for every one of my decisions. You completely failed to address them. I believe a code review should be a reasonable discussion based on valid criticism, not some kind of gatekeeper playground. If you can't do that, then you don't deserve contributions; for an open source project, that's a disgrace.

@iHiD
Copy link
Member

iHiD commented Dec 10, 2024

Hello 👋

Can we turn the heat down a little here please?


@atk The reason we ask for things to be decided on the forum before PRs are opened is because otherwise a huge proportion of conversations turn to shit just like this. Once someone has written code, it becomes harder for them to receive feedback that the core direction isn't what's desired. Exercism isn't a free-for-all where any contributions get in - we highly value contributions, and have tens of thousands of code-contributors, but if they don't work in the structure of how a maintainer is building a track, that tends to cause problems.

We need upfront discussion to understand the bounds of a track's exercise design and then for the contributor to decide if they want to work within that or not before investing time into contributing. We've found that that approach stops these issues from happening.

There's two blog posts on this:

A big part of a maintainer's job is to consider the setup of exercises as a whole - how they fit together and compliment each other. For people who are involved in the maintenance of multiple tracks, the scope of this widens further.

You describe that as "gatekeeping" but I disagree. Tracks that have consistent design philosophies tend to end up really well. Ones that don't tend to just be a mismatch of random ideas that don't really fit together. So I think the desire to keep things consistent across Assembly tracks, and to keep the track itself internally consistent are valid considerations and decisions that @keiravillekode has the right to make. However, those bounds should be laid out and clear to you before you do any work on them (thus the forum conversations)

I don't read any criticism at all in @keiravillekode's feedback. Instead I read that the way things have been implemented are not in keeping with the way the track is being built out. There's no suggestion that your code is wrong or bad, rather that it doesn't fit into the model that is being used across this track, and the assembly tracks.

presented arguments for every one of my decisions

Imagine that at track get 50 PRs. They all have different people with different opinions wanting different things. Now imagine a maintainer that's volunteering a few hours a week has to reply to all those PRs justifying everything and explaining why they see things differently. Nothing gets done. Everyone burns out. It's horrible. That's exactly what happened before (see blog post 1 above) and why we changed things. Nearly every maintainer we had (100s) gave up because its such an unenjoyable experience having people turn up and fight you over whether their philosophy/decisions are right / better / etc. It's just not a way of working that leads anywhere productive.


@keiravillekode What normally happens when a lot of the work has been done, but the track maintainers want to reshape things slightly, is that the track maintainers say "To accept this PR, I would like this to change. Would you like to do that [or shall I]?" (I put that bit in square brackets as the maintainer might not want to take that extra work on". Arbitrarily closing a PR is something that will lead to tension and that I wouldn't recommend. I understand that in this situation with @atk just "Resolving" your comments, maybe you felt that you'd asked for changes and they'd been rejected, so that this was done. But I think being more explicit about this (and calling me in earlier if needed) is wiser.

I also think being more explicit on your opinions not being a judgement of the code, but of alignment to track-standards/policies would be helpful and reduce the likelihood of someone taking your comments as criticism. A sentence of "Please don't take this as judgement of your code or its correctness, but it is important that the code adheres to the structure in which we're trying to shape these tracks and we value consistency over absolute correctness" might reduce potential tension before it arises and clarify things more clearly.


@atk If you'd like to tweak this to work within the bounds of the track, that would be very appreciated. I'm grateful for all the work you've put into Exercism and see no reason that we can't have a positive outcome of this PR. However, if you're done with this it and don't want to change things to be the way @keiravillekode wants, I totally respect that, but it means the PR will probably get closed (or tweaked by @keiravillekode as desired before merging).

@atk
Copy link
Author

atk commented Dec 10, 2024

I merely criticised that some of the requests seemed arbitrary with no further explanation, just to have my PR closed after questioning them instead of receiving answers. I have no qualms whatsoever changing this PR - or working together with the maintainers - but I do not want to spend time to change it into a far more limited solution merely for the sake of consistency with an exercise in a different track that doesn't even exist yet.

So let me propose a compromise that would work as an exercise in the way @keiravillekode intended it and as an actual game-of-life engine that you could add to your developer resumé page:

(module
  (memory (export "mem") 1)

  ;;
  ;; Create the next frame of Conway's Game of Life
  ;;
  ;; @param {i32} offset - The offset of the current frame stored as bit data in linear memory
  ;; @param {i32} length - The number of bytes in the frame
  ;; @param {i32} rowCount - The number of rows in the frame
  ;; @param {i32} columnCount - The number of columns in the frame
  ;;
  ;; @returns {(i32, i32)} - The offset, and length of the next frame with the same size in linear memory
  ;;
  (func (export "next") (param $offset i32) (param $length i32) (param $rowCount i32) (param $columnCount i32) (result (i32, i32))
    (return (local.get $offset) (local.get $length))
  )
)

I considered two other solutions:

  • prepending the frame with a dword for columnCount and rowCount: this would mix up data and arguments and would be inconsistent with other exercises
  • use the upper bit to mark the continuation of a row: this would increase the complexity of both encoding and decoding considerably - on the web assembly and JS side - it won't make for a very practical solution

This way both of us get what we want - the arbitrary size of the frame and the bit shift / mask exercise. Are you okay with that, @keiravillekode?

@keiravillekode
Copy link
Contributor

Yes, the proposed signature is certainly OK.

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