Skip to content

Add list-ops problem #170

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 3 commits into from
Oct 27, 2016
Merged

Add list-ops problem #170

merged 3 commits into from
Oct 27, 2016

Conversation

outkaj
Copy link

@outkaj outkaj commented Mar 25, 2015

I did not add the exercise to the config.json yet because I was not sure where it should be included in the order of problems.
This is my first time doing a pull request so please let me know if there are any issues.

@sjakobi
Copy link
Contributor

sjakobi commented Mar 25, 2015

Congrats on your first PR, Jacqueline!

That's a pretty cool exercise but it contains a few issues that should be fixed before it can be merged.

  • Please remove the __pycache__/list_ops.cpython-34.pyc file from the commit.
  • The _src suffix on the function names is a bit awkward. I'd propose that you simply use map, reduce and length instead, even if that means that the standard map function is shadowed (as well as reduce in Python2).
  • The Travis build will only succeed once the exercise is listed in config.json. We haven't really been very scientific about the order of the exercises so far. How about somewhere around the sieve exercise which is also about typical list functions?
  • All of the functions seem to accept arbitrary iterables so far. How about testing each of them with non-list iterables too, i.e. generators and/or ranges?
  • Maybe map should even be a generator (like the standard map function in Python3) instead of
    returning a list? What do you think?

I'll add a few more points as inline comments on the commited files.

@@ -0,0 +1,27 @@

Copy link
Contributor

Choose a reason for hiding this comment

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

It's only cosmetic but can you remove that line?

@outkaj
Copy link
Author

outkaj commented Mar 26, 2015

Thanks for all the helpful feedback. I will implement the changes you mentioned.
Putting list-ops near the sieve exercise makes sense.
Also, testing with generators and ranges is a good idea. I'll change two of the tests for reduce to reflect this and add two extra tests for length.
I'll try out map as a generator too, though wouldn't that make it Python3 specific?

@outkaj
Copy link
Author

outkaj commented Mar 26, 2015

I'm re-writing the tests for map() and am wondering what is the most efficient way to assert that a unit test returns a generator expression. Is there any way to compare two generator expressions without converting them both to lists or tuples?

@sjakobi
Copy link
Contributor

sjakobi commented Mar 26, 2015

I'll try out map as a generator too, though wouldn't that make it Python3 specific?

The Python3 map function returns an iterator while the Python2 map returns a list. In that sense the exercise may become a bit closer to Python3, which isn't a bad thing IMO. But Python2 has generators too, so a solution wouldn't be harder to write in Python2.

I'm re-writing the tests for map() and am wondering what is the most efficient way to assert that a unit test returns a generator expression.

While it can be done I somehow feel that it would be somewhat "unpythonic". It would certainly be in conflict with the concept of duck-typing. One idea would be leave this test to the nitpicking community. Another one may be to have a test that can only be practically passed with an implementation that returns a generator. For example ask for the sum for a large range of numbers or something like that.

Is there any way to compare two generator expressions without converting them both to lists or tuples?

Yes.

@outkaj
Copy link
Author

outkaj commented Mar 27, 2015

Thanks for responding. I agree that checking if a unit test returns a generator is unpythonic(/too Java-like...) I think your idea of a test that can only be passed with an implementation that returns a generator is a good one and I will try that out. That is also consistent with the idea of updating all three functions so that they accept various iterables.

@Dog
Copy link
Contributor

Dog commented Jun 5, 2015

Hi @outkaj,

Are you still working on this exercise? Do you need any help?

@behrtam
Copy link
Contributor

behrtam commented Nov 18, 2015

It is discussed right now to deprecate the accumulate problem in all tracks and use list-ops instead because both are very similar.

So @outkaj is there anything I can do to help you finish this?

@outkaj
Copy link
Author

outkaj commented Dec 8, 2015

Thank you both for checking in & sorry for the radio silence. I disappeared into other languages and projects, but will take a look at this right now and send a PR ASAP!

@outkaj
Copy link
Author

outkaj commented Dec 8, 2015

In other languages implementing this problem, I see additional functions such as filter, fold, append, reverse, and concatenate. I'd like to add them as well, since otherwise the Python equivalent of this exercise will be significantly easier by comparison. What do you think?

@behrtam
Copy link
Contributor

behrtam commented Dec 8, 2015

Good point! I would even go that far and split fold into foldr/foldl. Could nicely be tested with operations that are not commutative.

data = [1, 2, 3, 4, 5]

assert foldl(div, data, 1) == 0
assert foldr(div, data, 1) == 1
assert foldl(sub, data, 0) == -15
assert foldr(sub, data, 0) == 3

flat would also be a candidate for this exercise.

assert flat([[[1,2],[3]],[[4]]]) == [1, 2, 3, 4]

@outkaj
Copy link
Author

outkaj commented Dec 9, 2015

I just committed the updated version to my fork. Let me know how it looks and I can make the necessary changes before updating the PR. Relevant notes:
-I replaced reduce with foldl since they are very similar and also for syntax consistency with foldr. Problem description should probably clarify the meaning of the term fold and its similarity to built-in reduce.
-Use of autopep8 produced odd formatting at one point, which I fixed manually. I'm fairly certain I may still be violating some style guidelines. Is flake8 the recommended style checker/linter?
-Line 40 in example.py is there in error.

@behrtam
Copy link
Contributor

behrtam commented Dec 9, 2015

Some quick notes, will have a closer look later.

Yes we use flake8 as part of the travis-ci build. If you want every style error and warning:
flake8 . --select=E,W

I'm not sure if and how we should rename the bult-in functions filter and map to avoid confusion.

list-ops/example.py
Line 1: The reduce import could be removed now.

Line 6-10: len is already a bult-in, so I would rename the variable or rewrite the function

def length(xs):
    return sum(1 for x in xs)

Line 40: flat(["orange", "apple", "banana"]) probably a leftover from debugging

list-ops/list_ops_test.py
Line 4: should be from list_ops import ... or anything more specific than example

Line 42-43: I'm not sure about the reverse empty test. I would assume reverse([]) == []and not None.

Line 55, 57, 60, etc: you don't need lambda to pass a function as parameter

self.assertEqual(21, foldl((lambda x, y: operator.add(x, y)), [1, 2, 3, 4, 5, 6], 0))
self.assertEqual(21, foldl(operator.add, [1, 2, 3, 4, 5, 6], 0))

@outkaj
Copy link
Author

outkaj commented Dec 11, 2015

I would recommend re-naming filter and map. The original filter_src and map_src are too awkward, but how about filter_clone and map_clone?
I'll go ahead and make the other changes you suggested.

@outkaj
Copy link
Author

outkaj commented Dec 12, 2015

(I can squash the commits to just one reading "Add list-ops problem" once it's ready).

@behrtam behrtam mentioned this pull request Oct 27, 2016
@behrtam behrtam merged commit 296b21d into exercism:master Oct 27, 2016
behrtam added a commit that referenced this pull request Oct 28, 2016
Finishes the abandoned pr #170
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