Skip to content

[ Implementation ] OCR Numbers #750

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

Merged
merged 2 commits into from
Jun 24, 2025
Merged

Conversation

habere-et-dispertire
Copy link
Contributor

@github-actions github-actions bot closed this May 24, 2025
@kotp kotp reopened this May 25, 2025
@exercism exercism deleted a comment from github-actions bot May 25, 2025
Copy link
Member

@kotp kotp left a comment

Choose a reason for hiding this comment

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

With comments about test formatting, and potential clarification about lines.

); # end: 5efc9cfc-9227-4688-b77d-845049299e66

cmp-ok( # begin: f60cb04a-42be-494e-a535-3451c8e097a4
ascii-to-digits(" _ _ _ _ _ _ _ _ \n | _| _||_||_ |_ ||_||_|| |\n ||_ _| | _||_| ||_| _||_|\n "),
Copy link
Member

Choose a reason for hiding this comment

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

Especially for strings like this. However, I wonder if the spacing that is required would be hard to enforce, or easily deleted in maintenance.

That said, does it make sense that the last line in digit is defined as a line, meaning that it requires an EOL (\n) to be a valid line?

Copy link
Contributor Author

@habere-et-dispertire habere-et-dispertire May 25, 2025

Choose a reason for hiding this comment

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

Co-Pilot says we can't include external files for there is much ado. Here's an example of YAML failing if we try and use whitespace for input clarity:

        # template-data.yaml
        sprintf(q :to/END/, %case<input><rows>.join('\n').Str, %case<expected><error>.Str, %case<description>.raku);
          throws-like(
            { ascii-to-digits(
"%s"
            ) },
Generating ocr-numbers... Couldn't parse YAML

Do note that it is already in a heredoc, so I don't know if we can nest heredocs! Co-Pilot again says nay.

Nice catch on the final terminating newline.
I'll add it back in.

Copy link
Member

@kotp kotp left a comment

Choose a reason for hiding this comment

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

The commit message seems to be malformed. When I do git log --oneline it does not reveal helpful (to me) as I do not know the legend, can not decipher the communication here.

Once this is merged, the branch name benefit will go away, and all hope will be lost to understand in a summary view of the history.

@habere-et-dispertire
Copy link
Contributor Author

Once this is merged, the branch name benefit will go away, and all hope will be lost to understand in a summary view of the history.

I've tried to amend the commit text -- hopefully that works.

@kotp
Copy link
Member

kotp commented May 25, 2025

Once this is merged, the branch name benefit will go away, and all hope will be lost to understand in a summary view of the history.

I've tried to amend the commit text -- hopefully that works.

If you amend the commit, and that commit exists on the remote (hopefully not main/unbroken/master/whatever branch that is meant to be the immutable, add only branch) you would need to do a forced push to modify it.

If that is what happened, then it worked! I see two commits, but can only guess how they both came to be. I would welcome a state where there is now only one commit as this branch is your working branch, and you are free to change the history of it.

@habere-et-dispertire
Copy link
Contributor Author

If you amend the commit, and that commit exists on the remote (hopefully not main/unbroken/master/whatever branch that is meant to be the immutable, add only branch) you would need to do a forced push to modify it.
If that is what happened, then it worked!

I did do a forced push.
Would you be happier with a fresh PR to make a cleaner history ?
It's no bother.

@kotp
Copy link
Member

kotp commented May 25, 2025

If you amend the commit, and that commit exists on the remote
(hopefully not main/unbroken/master/whatever branch that is meant to
be the immutable, add only branch) you would need to do a forced push
to modify it.
If that is what happened, then it worked!

I did do a forced push. Would you be happier with a fresh PR to make a
cleaner history ? It's no bother.

No, no need, you can do an interactive rebase and squash the commits
together, if you want to, and then force push. We will see the result
of whatever history you want to present.

No need for starting the pull request process over, the patch will end
up being whatever the diff is from main to your branch.

@habere-et-dispertire
Copy link
Contributor Author

@kotp does that look any better ?
I tried to merge the four commits .

@kotp
Copy link
Member

kotp commented May 27, 2025

@kotp does that look any better?
I tried to merge the four commits .

It looks beautiful!

asciicast

Copy link
Member

@kotp kotp left a comment

Choose a reason for hiding this comment

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

I like the way the tests read now, I hope you do too!

I do not get the "ROAST" delimiter, but appreciate it. I wonder how many students will question this choice of delimiters, but it also shows that the delimiter does not have to be END, so there is definitely value in that.

@habere-et-dispertire
Copy link
Contributor Author

I do not get the "ROAST" delimiter, but appreciate it.

Humour appears in scope when it comes to naming in raku !
The official raku test suite is named roast . 🍠

@habere-et-dispertire habere-et-dispertire marked this pull request as draft May 27, 2025 05:45
@kotp
Copy link
Member

kotp commented May 27, 2025

I do not get the "ROAST" delimiter, but appreciate it.

Humour appears in scope when it comes to naming in raku ! The official raku test suite is named roast . 🍠

OK, I mean, I get the association to the test framework, but these are expectations, not an entire test, or a single roast. The name slips a little here, maybe. So maybe TASTE or FLAVOR or something regarding roasts, but not an entire roast.

Still, does not matter. Humor is tough (unlike your lovely roasts, I am sure).

@habere-et-dispertire habere-et-dispertire marked this pull request as ready for review June 1, 2025 11:47
@kotp kotp merged commit 70d4888 into exercism:main Jun 24, 2025
4 checks passed
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.

2 participants