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 Practice Exercise: ledger #196

Open
0xNeshi opened this issue Sep 6, 2024 · 8 comments · May be fixed by #316
Open

Add Practice Exercise: ledger #196

0xNeshi opened this issue Sep 6, 2024 · 8 comments · May be fixed by #316
Assignees
Labels
x:action/create Work on something from scratch x:module/practice-exercise Work on Practice Exercises x:rep/large Large amount of reputation x:type/coding Write code that is not student-facing content (e.g. test-runners, generators, but not exercises) x:type/content Work on content (e.g. exercises, concepts)

Comments

@0xNeshi
Copy link
Contributor

0xNeshi commented Sep 6, 2024

Problem specification:
https://github.com/exercism/problem-specifications/tree/main/exercises/ledger

TODO:

@0xNeshi 0xNeshi added x:action/create Work on something from scratch x:module/practice-exercise Work on Practice Exercises x:size/medium Medium amount of work x:type/coding Write code that is not student-facing content (e.g. test-runners, generators, but not exercises) x:type/content Work on content (e.g. exercises, concepts) x:rep/large Large amount of reputation and removed x:size/medium Medium amount of work labels Sep 6, 2024
@Falilah
Copy link
Contributor

Falilah commented Dec 4, 2024

can I take this?

@0xNeshi
Copy link
Contributor Author

0xNeshi commented Dec 4, 2024

This exercise is a bit different than the others - the idea is to give students a working, but complex solution, and have them refactor it to make it as simple as possible.

This last sentence from the exercise instructions:

Please keep a log of what changes you've made and make a comment on the exercise containing that log, this will help reviewers.

applies to this issue's PR as well, i.e. the PR you create should contain a description/comment explaining what changes you made to the complex solution (lib.cairo) to make it simpler (example.cairo). We will use this as a good example of a refactor to show students who might ask their mentors to review their work.

Let me know if anything is unclear or if you need help!

@Falilah
Copy link
Contributor

Falilah commented Dec 4, 2024

Alright, I will look into it properly and get back if I have any question. Thanks

@Falilah
Copy link
Contributor

Falilah commented Dec 11, 2024

Hi @0xNeshi, the symbol is not an ascii character so I am replacing it with eur. I do not know if that is accepted or if you can help with an implementation that can help print the symbol nicely.

And also I have the passing code in a messy and complex manner, do not know if it will be acceptable and the ledger is printed in the order in which it is being passed in the arguement and not ordered by the date of the transaction.

@0xNeshi
Copy link
Contributor Author

0xNeshi commented Dec 11, 2024

Since Cairo only supports ASCII chars, so must we in this exercise. Let's use the lowercase letter e instead of , to keep currency lengths consistent, and to avoid formatting complications in the implementation. Just make sure to add a short instructions.append.md file that explains why we made this change in our exercise.

And also I have the passing code in a messy and complex manner, do not know if it will be acceptable and the ledger is printed in the order in which it is being passed in the argument and not ordered by the date of the transaction.

The most important thing is for the test cases from canonical data to pass with the exact inputs and outputs (or as close to exact as possible, in our case using e instead of ). Only after that do we worry about the "cleanliness" and readability of the implementation. So feel free to do it the "messy" way 👍

@Falilah
Copy link
Contributor

Falilah commented Dec 11, 2024

In the canonical.json test 4 have the same slug as the last test, but looking at their cases they are not the same, the test 4 tested for transactions on different dates while the other tested for transactions on the same date.

Even thou the last test has an additional comment in the test.toml file stating it is reimplementing test 4. I still feel it is not and the name for the test 4 should be changed to multiple_entries_on_different_date_ordered_by_description instead of multiple_entries_on_same_date_ordered_by_description.

cairo won't let me declare more than one test with the same name, so I had to change the test 4 to the name I suggested above.

what do you think?

@BNAndras
Copy link
Member

A test marked as reimplementing another one is intended to replace it completely in the test suite. I wouldn’t expect both to be present so having the same name shouldn’t occur.

@Falilah
Copy link
Contributor

Falilah commented Dec 12, 2024

Okay well noted, I can see now that the number 4 has the include = false comment. so it safe to remove it from my test cases.
Thank you very much.

@Falilah Falilah linked a pull request Dec 12, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
x:action/create Work on something from scratch x:module/practice-exercise Work on Practice Exercises x:rep/large Large amount of reputation x:type/coding Write code that is not student-facing content (e.g. test-runners, generators, but not exercises) x:type/content Work on content (e.g. exercises, concepts)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants