-
Notifications
You must be signed in to change notification settings - Fork 163
Conversation
def test_clean_up(self): | ||
|
||
input_data = ( | ||
("Scrimshaw Whale's Tooth", '2A', 'Deserted Docks', ('2', 'A'), 'Blue'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nitpick: there's inconsistency between this line (which uses double quotes for this string literal) and other lines (which use single quotes, and escape the embedded apostrophe). I recommend making the others match this to avoid escaping quotes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup. Thank you for catching that! The quotes were a nightmare when it came to checking results from std.out
. Next time, I won't name something with an apostrophe int it! 😆 Fixed in latest push.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find running black
on my code to be useful. Among other minor style changes, it would've converted all strings to use double quotes, unless they contain a double quote, then it uses single quotes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. Somehow, in my long twisty path I've developed a habit of single quotes. It may be time to remedy that .. and to set up Black to work with Pycharm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In VS Code, I have flake8
run on file save and alert me of violations, then I manually run black
to fix the violations it can. I don't like editors modifying my files implicitly.
I don't use PyCharm, but I'm sure there's a way to set up something similar.
…d cleaned up bloat.
…as learning objective.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@BethanyG Thanks a ton for working on this! This is a great first exercise PR, congrats! You said you'd like a thorough review, so I've obliged :)
Note that I've approved this PR so feel free to merge this and apply and feedback comments in a follow-up PR.
finally: | ||
sys.stdout, sys.stderr = old_out, old_err | ||
|
||
class TuplesTest(unittest.TestCase): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By default, all tests currently run. One of the things our test suites aim for is to have the student solve the exercise in a TDD-like way, where you focus on getting one test passing at a time.
Writing this, I wanted to point you to the existing v2 Python test suites for an example, but it turns out that none of the tests are skipped there (see the bob
tests or change
tests). An an example of what a TDD-like test suite looks like, see this C# test suite or this Elixir test suite.
This might be something to discuss with the other Python maintainers, but I assume there is some good reason for it (ping me in the discussion as I'm very interested). It does look like pytest allows skipping tests: https://docs.pytest.org/en/latest/skipping.html#skipping-test-functions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's the discussion on that:
exercism/python#1853
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see. If I read that discussion correctly, the main reason for not using skipped tests is that the canonical data ordering might not be the TDD ordering that is appropriate to Python, right? If so, that would mean that you could do skipped tests for Concept Exercises, as the ordering of the tests is completely up to the Python maintainers. Right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is correct.
# alt return coordinate[0], coordinate[1] | ||
return tuple(coordinate) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest only having one option here (the most idiomatic one), as this file will be shown to mentors and having two options might be confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both are idiomatic (for better or worse). But since we're trying to get the student to practice different ways of making tuples, I'll re-read in the am and make sure I have one task that uses one method, and another that uses the constructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return (coordinate[0], coordinate[1])
seems most natural to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll re-read in the am and make sure I have one task that uses one method, and another that uses the constructor.
Brilliant!
def clean_up(combined_record_group): | ||
for item in combined_record_group: | ||
clean_record = item[0], item[2], item[3], item[4] | ||
print(clean_record) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this print to the console? If so, could you convert this to returning a value (or modifying the input) instead of printing to the console? In general, exercises don't print to the console as that makes writing the tests harder and will interfere with capturing meaning console output for in-browser debugging purposes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does indeed print to the console. Not at all ideal, but I'm struggling here because tuples are immutable. The point of this last task was to iterate through a tuple and do something with the data (in this case 'dropping' an element from each 'record' within the outer tuple) -- but making a new tuple of tuples iteratively via the + operator means that the data gets unpacked -- so the nested tuples become un-nested. 😞
Alternatively, the inner tuples could be added into a list
that is then converted -- but lists are neither required / taught for this exercise. The input could be changed to something weird like double-wrapped tuples (three layers of nesting as opposed to two) so that the + doesn't completely unpack them - but that seems very contrived. Maybe if this required a tuple of each team members records as input with the output being a combined tuple....
Hum. Suggestions here? I will sleep on it and try to think of an alternate first thing in the morning. 😁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The point of this last task was to iterate through a tuple and do something with the data
Maybe outputting the combined records in a new string format? Would something like that work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe outputting the combined records in a new string format? Would something like that work?
This is similar to what we do in the tournament
exercise, and would be preferable here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! Making a string and compiling a 'report' seems like a much better solution - and strings
are required knowledge for this exercise, so something with .format()
or f-strings
in a report would work well. I'll take a look at tournament
and how the instructions read there, and change this task accordingly. thank you both for getting me un-stuck! 😀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our pleasure!
@@ -0,0 +1,145 @@ | |||
In Python, a tuple is an immutable collection of items in _sequence_. Like most collections, tuples can hold any (or multiple) data type(s) -- including other tuples. Like any sequence, items are referenced by 0-based index number. Tuples support all [common sequence operations](https://docs.python.org/3/library/stdtypes.html#common-sequence-operations). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a resource to link to for the tuple
concept?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added in https://github.com/exercism/v3/blob/master/languages/python/reference/concepts/builtin_types/tuple.md
, which needs additional info. In particular, the whole homogenous vs heterogeneous data (as opposed to data type) is very confusing to most people. I have a note to add clarification links to docs and articles there - or generally more explanation.
@@ -0,0 +1,145 @@ | |||
In Python, a tuple is an immutable collection of items in _sequence_. Like most collections, tuples can hold any (or multiple) data type(s) -- including other tuples. Like any sequence, items are referenced by 0-based index number. Tuples support all [common sequence operations](https://docs.python.org/3/library/stdtypes.html#common-sequence-operations). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like this after.md
document!
…d introduction.md file name spelling.
…ts and code examples.
…()-ing of content. Adjusted instructions accordingly.
… context handler decorator for re-routing stdout and extra imports.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent work! The final thing to do is to add an entry to the track's config.json
file: https://github.com/exercism/v3/blob/master/languages/python/config.json
Mashing big, green button. 😱 |
Related Issue: #1097.
New tuples concept exercise.
Since this is my very first and I didn't use the spiffy exercise creatormatic, would really like a through critique. 😄