Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Conversation

@josix
Copy link
Contributor

@josix josix commented Oct 26, 2017

resolve issue #1012

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.

@wilson8507 I've left some comments, please address them.

self.example.search('ecmascript'),
(Point(9, 0), Point(9, 9))
)
self.assertEqual(
Copy link
Contributor

Choose a reason for hiding this comment

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

There are a lot of redundant and misplaced assertions. Please check that each assertion actually belongs under its test method (Example: checks for "clojure" in self.example should only appear in tests for horizontal words).

josix added 3 commits October 27, 2017 01:33
- Delete assertions that appearing in multiple test methods
- Make sure each test case appearing in correct test methods
@josix
Copy link
Contributor Author

josix commented Oct 26, 2017

@cmccandless I've fixed it, please check if there is any problem. Thanks!

from word_search import WordSearch, Point

# Tests adapted from `problem-specifications//canonical-data.json` @ v1.1.0

Copy link
Contributor

Choose a reason for hiding this comment

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

Minor formatting point, but can you move the version comment down a line (ie. two blank lines before, one after).

Copy link
Contributor

@N-Parsons N-Parsons left a comment

Choose a reason for hiding this comment

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

I've left a comment regarding a minor formatting issue. Once you've resolved it, I'm happy merge this.

Thanks for the hard work, @wilson8507!

@N-Parsons N-Parsons self-assigned this Oct 26, 2017
@josix
Copy link
Contributor Author

josix commented Oct 27, 2017

@N-Parsons I've resolved it. Thank you for pointing out my carelessness!

@N-Parsons
Copy link
Contributor

Perfect! Thanks, @wilson8507! I'm a bit picky about formatting and consistency 😛

@N-Parsons N-Parsons merged commit f0dfd60 into exercism:master Oct 27, 2017
josix added a commit to josix/python that referenced this pull request Oct 28, 2017
)

* word-search: Update tests to version 1.1.0(exercism#1012)

* Remove redundant and misplaced assertions
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.

4 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.