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

Switch to assertj for exceptions #2502

Merged

Conversation

keszocze
Copy link
Contributor

@keszocze keszocze commented Sep 28, 2023

pull request

Switching to AssertJ for the

  • Forth
  • Ledger
  • OCR
  • Palindrom Calculator
  • Phone Number
  • SGF Parsing
  • Simple Linked List
  • Triangle
    exercises.

This request brings #2147 closer to being fully solved.


Reviewer Resources:

Track Policies

@sanderploegsma
Copy link
Contributor

It looks like this includes updates to error-handling, which was already merged. Could you rebase your branch on exercism:main?

@keszocze
Copy link
Contributor Author

The change is very tiny in the sense that I only make the imports explicit. I probably can rebase it but have to look up what that is and how it works. I am wondering whether that really is worth the hassle, though.

@sanderploegsma
Copy link
Contributor

Ah well the reason I mentioned it is because there are conflicts with the main branch.

If rebasing is new for you it’s also fine if you just merge the main branch into yours and resolve the conflicts that way 👍

@sanderploegsma
Copy link
Contributor

sanderploegsma commented Sep 29, 2023

@keszocze thanks for picking up all of these! I'd like to ask you to make additional updates in a different PR: updates to exercise tests trigger new test runs of submitted solutions on the Exercism infrastructure, so we try not to update too many at once at risk of overloading the servers.

Plus, small and focused PRs make my job of reviewing them a bit more manageable 🙃

@keszocze
Copy link
Contributor Author

Sure can do that. Then I stop adding more to this PR. As all changes belong to a single issue, I thought that this was fine. I sure do not want to make your life unnecessarily complicated ;)

Copy link
Contributor

@sanderploegsma sanderploegsma 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 so far! I have left a couple of comments for you to address before we can merge this.

Copy link
Contributor

@sanderploegsma sanderploegsma left a comment

Choose a reason for hiding this comment

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

All good, thanks for contributing!

@sanderploegsma sanderploegsma merged commit 529b75a into exercism:main Sep 29, 2023
3 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