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

Implement checks for raising messages with exceptions #1113

Merged
merged 7 commits into from
Dec 12, 2017

Conversation

N-Parsons
Copy link
Contributor

@N-Parsons N-Parsons commented Nov 18, 2017

Resolves #1080 by adding checks that all exceptions are raised with messages. The aim is to encourage learners to raise meaningful messages, but testing for meaningfulness isn't feasible, so this instead just tests that a message of non-zero length is passed when raising an exception.

TODO:

  • Update any failing examples
  • Add section to the README insert to explain the need for meaningful exception messages
  • Regenerate READMEs to include the new insert

@N-Parsons N-Parsons force-pushed the raise-messages branch 5 times, most recently from b7dc4e3 to ab1acd7 Compare November 18, 2017 23:15
@N-Parsons N-Parsons changed the title [WIP] Implement checks for raising messages with exceptions Implement checks for raising messages with exceptions Nov 19, 2017
@N-Parsons
Copy link
Contributor Author

N-Parsons commented Nov 19, 2017

I've held back the change to the proverb exercise description due to significant changes and put these in a separate PR (#1114) with the associated changes to the test-suite. This means that there will be a merge conflict, but I should be able to quickly resolve this once one is merged. (EDIT: Conflict resolved)

Copy link
Contributor

@cmccandless cmccandless 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 can confirm that most/all changes to README files (besides the insertion of the new Exception Messages section) are simply reflecting the latest versions of the relevant problem descriptions, then LGTM.

@N-Parsons
Copy link
Contributor Author

@cmccandless, all changes reflect changes to the upstream problem descriptions. I had a look through and nothing significant appeared to be changing (except in proverb, hence #1114), so the changes shouldn't cause issues.

Nathan Parsons added 6 commits November 21, 2017 16:26
(Fixes exercism#1080)

* Add self.assertRaisesWithMessage method to relevant exercise tests
    - Uses self.assertRaisesRegex
    - Checks only for the presence of a message, not content
* Add meaningful messages to failing examples
* octal: Switch to using a context manager for exception tests
@stale
Copy link

stale bot commented Dec 12, 2017

This issue has been automatically marked as abandoned because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the abandoned label Dec 12, 2017
@stale stale bot removed the abandoned label Dec 12, 2017
@cmccandless cmccandless merged commit f53e2ef into exercism:master Dec 12, 2017
@N-Parsons N-Parsons deleted the raise-messages branch January 18, 2018 17:49
@cmccandless cmccandless mentioned this pull request Feb 15, 2018
77 tasks
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