Skip to content

Conversation

@homersimpsons
Copy link
Contributor

@homersimpsons homersimpsons commented Jan 29, 2024

This is the first time I add an exercise. I followed the walkthrough at https://exercism.org/docs/building/tracks/practice-exercises.

Do not hesitate to tell me if anything is wrong. I may try to implement other exercises so better be right from the beginning.

Some notes:

  • I inspired myself from the javaScript track https://github.com/exercism/javascript/blob/main/exercises/practice/list-ops, but they are creating a "class" to represent the string, on this part I decided to stick closer to my understanding of the instructions and followed what the java track did https://github.com/exercism/java/blob/main/exercises/practice/list-ops/src/main/java/ListOps.java (Note that the Go track also takes the "class" approach too)
  • for the example.php I tried to follow the instructions (not using std's functions), so maybe I could also put it as exemplar (with some added phpdoc)
  • Should I add template documentation for the callable received in filter, map, foldl and foldr ? => Added to ease completion of the exercise
  • Should I update the instructions ? => Added an "append" file to add some context around callables
  • The PHPCS warnings are for line length in @testdox comments, how should I go to fix them ? => I do not think it is relevant to fix them here
  • There is no task for this exercise, should I create them ? => I may do it in an up-comming Pull Request, but I do not think it is that necessary
  • I used configlet to generate the exercise, it looks like this created a huge diff to config.json should I revert it to only add the relevant part for list-ops ? => I kept the diff as small as possible here
  • Which level of difficulty is this exercise ? => Hard (7)

@neenjaw I put you as a reviewer as it looks like you are the maintainer for the exercises.

@mk-mxp
Copy link
Contributor

mk-mxp commented Jan 30, 2024

Thanks for your contribution! I'm not a maintainer, but have opinions on some of your questions.

Should I add template documentation for the callable received

Let students figure that out to add to the difficulty. Or reduce difficulty by providing it. Both options are OK.

Should I update the instructions ?

Please don't. They are synced to the problem specifications. If you would like to add PHP specific instructions (like the tasks you suggested), you can add exercises/practice/list-ops/.docs/instructions.append.md.

The PHPCS warnings are for line length

I suggest to generally suppress warnings by adding -n to the command in the shell scripts. Or adding <arg value="n" /> to phpcs-php.xml. Or merging #591, which also would allow to drop the huge comments about strict types in example solution and tests.

I used configlet to generate the exercise, it looks like this created a huge diff to config.json should I revert it to only add the relevant part for list-ops?

I would recommend doing so. Or better: Reformat config.json in another MR, e.g. using configlet fmt.

Which level of difficulty is this exercise ?

I see it as hard. The track has no (I haven't seen any, yet) static methods around, only a planned concept about it. Functional programming as such is not very common in this track, too. I haven't seen a concept for anonymous functions nor use of them in examples, yet. So, hard would be my choice.

@homersimpsons homersimpsons force-pushed the exercise-list-ops branch 2 times, most recently from cee862e to c0b5270 Compare January 30, 2024 23:20
@homersimpsons
Copy link
Contributor Author

Thank you for your feedback @mk-mxp !

Let students figure that out to add to the difficulty. Or reduce difficulty by providing it. Both options are OK.

Okay, I added it so it should be easier to learn things thanks to this exercise.

Please don't. They are synced to the problem specifications. If you would like to add PHP specific instructions (like the tasks you suggested), you can add exercises/practice/list-ops/.docs/instructions.append.md.

Okay, I did add a file to explain "callable". I don't intend on adding tasks for now at least.

I suggest to generally suppress warnings by adding -n to the command in the shell scripts. Or adding to phpcs-php.xml. Or merging #591, which also would allow to drop the huge comments about strict types in example solution and tests.

I think you approach is right. I will let it for another Pull Request.

I see it as hard. The track has no (I haven't seen any, yet) static methods around, only a planned concept about it. Functional programming as such is not very common in this track, too. I haven't seen a concept for anonymous functions nor use of them in examples, yet. So, hard would be my choice.

Okay, I put it at 7 for now. This may be too high though. But in fact if none of the concepts are explained earlier it make sense.

@homersimpsons
Copy link
Contributor Author

homersimpsons commented Feb 15, 2024

Just thinking @mk-mxp, maybe I can just remove the class thing and have the methods in the global scope, that would make the exercise way easier.

Edit: I just removed the static. I saw that nealy all of the exercise have their functions embeded in a class.

@homersimpsons
Copy link
Contributor Author

list-ops is the next week exercise, does anyone have time to review this ?

I can merge it, but I would prefer a review before as this is the first time I contribute a full exercise. Once this is merged I will most likely address all the remaining exercises from 48in24 for the php track.

@mk-mxp mk-mxp added x:knowledge/elementary Little Exercism knowledge required x:module/practice-exercise Work on Practice Exercises x:size/large Large amount of work x:rep/large Large amount of reputation labels Feb 15, 2024
@mk-mxp
Copy link
Contributor

mk-mxp commented Feb 15, 2024

@homersimpsons We will get that merged in time. I think I need too long to get #591 merged. Could you add <arg value="n" /> to phpcs-php.xml, so the CI is fixed and re-run?

I'll look at your code again this weekend. If I remember correctly there were no show stoppers in it.

@homersimpsons
Copy link
Contributor Author

Could you add to phpcs-php.xml, so the CI is fixed and re-run?

done

Copy link
Member

@ErikSchierboom ErikSchierboom left a comment

Choose a reason for hiding this comment

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

Nice work!

@mk-mxp mk-mxp removed the request for review from neenjaw February 16, 2024 18:46
Copy link
Contributor

@mk-mxp mk-mxp left a comment

Choose a reason for hiding this comment

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

If you do not want to change the exercise as suggested, I'm OK with merging it as is. Just tell me your choice.

Thanks for contributing!

}

/**
* @testdox append entries to a list and return the new list -> empty lists
Copy link
Contributor

Choose a reason for hiding this comment

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

I love this! It looks even better with a capital letter at the beginning of the sentence. And I suggest adding the UUID of the test case, like here:

Suggested change
* @testdox append entries to a list and return the new list -> empty lists
* @testdox Append entries to a list and return the new list -> empty lists
* uuid: 485b9452-bf94-40f7-a3db-c3cf4850066a

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just copy-pasted those from the JavaScipt track.

The @testdox matches the description in the tests.toml file, do you think I should still diverge from this description and use a capital letter ?

Copy link
Contributor

Choose a reason for hiding this comment

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

The description in the problem spec is written to be used like sprintf('It %s', $description). We have no testing framework, that does so (like jest in JavaScript or TypeScript). That's why I think, we should adjust it to our needs. It looks a bit misplaced in the online editor when not Capitalised. Try it in Lucky Numbers vs. City Office, for example. Open the successful test result and see the difference.

Copy link
Contributor Author

@homersimpsons homersimpsons Feb 17, 2024

Choose a reason for hiding this comment

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

The description in the problem spec is written to be used like sprintf('It %s', $description).

So maybe we should update this is the test runner instead ? So we can keep this consistent with the toml.

Reference: https://github.com/exercism/php-test-runner/blob/61252274d05daf021fa70c6166edc1b9a0d1ce63/junit-handler/src/Handler.php#L160

Or you prefer we do this on a test by test basis ?

We have no testing framework, that does so (like jest in JavaScript or TypeScript)

https://pestphp.com/ does this, but I personally prefer PHPUnit. (pest is using phpunit under the hood).

Copy link
Contributor

@mk-mxp mk-mxp Feb 17, 2024

Choose a reason for hiding this comment

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

I prefer having that in the test. Because this makes PHPUnit output the testdox nicely on the command line when working with the CLI.

I put some hope onto tomasnorre's test generator to take care of that. But for now, I would be happy if you do it. I will merge with or without capitals...

@homersimpsons
Copy link
Contributor Author

Note that the job "Configlet Generate / configlet-generate (pull_request)" is failing because it is trngi to pull the branch from this repository instead of from my fork ...

@mk-mxp
Copy link
Contributor

mk-mxp commented Feb 17, 2024

Note that the job "Configlet Generate / configlet-generate (pull_request)" is failing because it is trngi to pull the branch from this repository instead of from my fork ...

I need to investigate, what configlet generate changes and wants to commit. I haven't expected that.

@mk-mxp
Copy link
Contributor

mk-mxp commented Feb 17, 2024

Note that the job "Configlet Generate / configlet-generate (pull_request)" is failing because it is trngi to pull the branch from this repository instead of from my fork ...

@homersimpsons So you have been faster than me typing...anyways, the problem was:

Your local branch main needs to be updated to match exercism/php:main, then you must rebase your exercise-list-ops branch onto your main. configlet compares your /exercises/concept/lasagna/.docs/introduction.md to the current exercism/php:main and sees the need for an update. That's what the CI tries to commit, but that must not be done.

@mk-mxp mk-mxp merged commit b80fc00 into exercism:main Feb 17, 2024
@mk-mxp
Copy link
Contributor

mk-mxp commented Feb 17, 2024

Thanks for contributing! Very nice work...

@homersimpsons homersimpsons deleted the exercise-list-ops branch February 17, 2024 19:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

x:knowledge/elementary Little Exercism knowledge required x:module/practice-exercise Work on Practice Exercises x:rep/large Large amount of reputation x:size/large Large amount of work

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants