Generated Custom Set by kotp · Pull Request #383 · exercism/ruby

kotp · 2016-06-22T17:21:03Z

The custom-set exercise is generated using data from x-common
repository.

fixes #365
fixes #348
references exercism/problem-specifications#257

Cohen-Carlisle · 2016-06-23T03:34:46Z

 require 'minitest/autorun'
 require_relative 'custom_set'

+# rubocop:disable Metrics/LineLength


Hmm, I don't like this. The offending lines seem overly verbose. Would it be advisable to shorten them up in the x-common json?

Yes, it does make sense to attack this in the common file. I did not like it when I did it either. I am not addressing it here though, as it is not the 'source'. The source is the linked issues, I think I have linked them in the PR.

This file is generated and the lines are only long because the test names are verbose so I have no problem with disabling the LineLength metric here.

Insti · 2016-06-23T07:31:49Z

+    "set1 = CustomSet.new #{set1}
+    set2 = CustomSet.new #{set2}
+    expected = CustomSet.new(#{expected})
+    #{assert_or_refute}_equal set1.union(set2), expected"


I like how you're assigning expected to a variable, but it appears that you have expected and actual inverted which will cause confusing test failure messages.
(A lot of the test templates in this file have this issue.)

The signature for assert_equal is: assert_equal(exp, act, msg = nil)

kotp · 2016-06-23T08:06:26Z

+  end
+
+  def test_body
+    send section


I was trying to attack this vector from the other end... This is much simpler... Thanks @Insti !

Cohen-Carlisle · 2016-06-24T01:02:42Z

+  json = JSON.parse(data)
+  cases = []
+  (json.keys - ['#']).each do |section|
+    json[section]['cases'].each_with_index do |row, i|


I believe this each_with_index should be simply an each while the above line's each should be the each_with_index. This is currently leaving the first exercise of each type (diff, union, etc.) unskipped.

@Cohen-Carlisle Thanks for keeping an eye on this...

Cohen-Carlisle · 2016-06-24T01:03:47Z

Noticed one more thing about skips but other than that I think it looks good.

kotp · 2016-06-24T03:15:20Z

Manual index had to go back in, if that is the only thing with the skips, I think we are ready to merge.

Cohen-Carlisle · 2016-06-24T17:02:43Z

+    skip
+    set = CustomSet.new [1]
+    refute_equal CustomSet.new, set
+  end


Shouldn't the empty tests use the empty? method? As written, these are really testing equality and not emptiness.

The test assert_empty? requires that the object responds to empty? and there is no spec for that currently.

I believe this should be that very spec. The metadata for these exercises is:

"empty": { "description": "Returns true if the set contains no elements", "cases": [ "..." ] },

This is not the place to rely on equality. Equality has its own section later on.

Cohen-Carlisle · 2016-06-24T17:27:48Z

Looked more closely and had more comments 😛

Cohen-Carlisle · 2016-06-26T15:35:12Z

There seems to be some confusion over whether to use empty?. In my mind, there is no question. The metadata intends for the first exercises to have an empty? method first and much later to implement an equality method.

kotp · 2016-06-27T04:23:03Z

If I am following the leads properly, it seems that there no common data that dictates a test that forces a method to be defined strictly named "empty?". That is an implementation that may be wise, but at the moment I am not seeing that it is forced. The same with equality.

The metadata that I am looking at is specifically 'example.rb', and 'custom-set.json', and nothing else. The restriction is "refactoring" level, so no changes in behavior, hopefully. The goal for this PR is simply to bring in the tests from x-common repository. Definitely not to make improvements in the testing at this time.

I may not be looking at it from the proper point of view though.

If I have accomplished this, with this goal in mind, then I think we can bring it in, and then address the "we should have the implementation have empty? and equals defined in the track" for this problem set.

I think you are correct about the equality and the empty being defined... and an "empty?" method being desirable. Especially since "empty" is defined, and so a query partner for that method makes even more sense, because of it.

Insti · 2016-06-27T15:12:21Z

There should not be an empty? method.

There is an empty section in custom-set.json but there are also a lot of other sections in that file that act as descriptions of what is tested within that section and not specifications of query methods.

The tests that @kotp has implemented accurately reflect the intent of the common data and the problem description.

Cohen-Carlisle · 2016-06-27T17:26:21Z

I strongly disagree. Looking at the PR that put the current ordering into place (exercism/problem-specifications#257), we see the comment:

The goal of this change is to ensure that no tests that require set equality in an assertion precede the tests for set equality. This allows each set of tests to introduce as little new functionality as possible (assuming that tests are completed from top to bottom).

Edit: Tagging @ryanplusplus for make him aware there is still confusion over the appropriate time to use equality, and for any input.

Insti · 2016-06-27T22:09:26Z

I was wrong.

Having re-read the referenced thread and reviewed the json file I now agree with @Cohen-Carlisle
and believe an empty? method is required.

Insti · 2016-06-27T22:12:09Z

+    skip
+    set = CustomSet.new []
+    element = 1
+    refute set.member? element


member? should be contains?

member is not referenced in the custom-set.json but contains is.

I would actually make an argument for include? as its the most common duck typed method for this kind of thing in Ruby. I don't think we have to stick to the description exactly, especially when ruby has a clear pattern for this kind of query.

Edit: And include? is the Enumerable standard. http://ruby-doc.org/core-2.3.1/Enumerable.html#method-i-include-3F

The names in section don't necessarily have to match the methods, they are section headers for common-data. But I changed it in the example code anyway.

Cohen-Carlisle · 2016-06-28T02:30:57Z

Two of my comments seem to have been buried after unrelated diff changes.

The subset? tests are backwards. For example, we have a test that reads like this when variables are in-lined:

    assert CustomSet.new([1]).subset? CustomSet.new([])

Read in English, this reads:

assert that a set with the element 1 is a subset of an empty set

Which is actually backwards. Ruby's standard library agrees.

I favor the use of include? over contains? as include? is the ruby standard for this. Illustrated in Enumerable and Array.

Cohen-Carlisle · 2016-06-28T02:33:03Z

Also, #386 applies here. Not sure if we want to hold it up over that or fix it later.

kotp · 2016-06-28T07:09:51Z

I think it was taken care of, if this is the code that you are thinking of.

  def test_empty_set_is_a_subset_of_non_empty_set
    skip
    set1 = CustomSet.new []
    set2 = CustomSet.new [1]
    assert set2.subset? set1
  end

  def test_non_empty_set_is_not_a_subset_of_empty_set
    skip
    set1 = CustomSet.new [1]
    set2 = CustomSet.new []
    refute set2.subset? set1
  end

Insti · 2016-06-28T11:39:17Z

@kotp I'm confused.
The code you have quoted looks wrong to me.
In test_empty_set_is_a_subset_of_non_empty_set set2 is not a subset of set1
I think this is what @Cohen-Carlisle is also saying.
Using include? seems reasonable, I don't have particularly strong opinions on method naming in this case. It is nice to know that matching the names in the json is not required.

kotp · 2016-06-28T12:11:20Z

When you use a named test in minitest it can use a specific method. When you use equals, then you use whatever you want to compare together. This is why using them will sometimes expose that a certain method is not used, due to internals, at least that is what I am seeing when I apply suggested changes with specific method names.

In the code that appears wrong, feel free to clone the PR branch and play with it, and propose a solution... because I am not seeing what you are seeing, or I am not properly recreating the situation. I have changed it over and over again, and get feedback based on the actual runs.

Cohen-Carlisle · 2016-06-30T02:52:48Z

I'm unfortunately pretty busy right now and will be for a while.

I was trying to say that the logic around the receiver and argument should be swapped. Instead of Set[1].subset? Set[] # => true, it should be Set[].subset? Set[1] # => true.

In other words, it should be implemented as "is the receiver a subset of the argument?", while its currently "is the argument a subset of the receiver?"

kotp · 2016-06-30T07:29:55Z

I had changed it I think at least twice now... Going from

  def test_empty_set_is_a_subset_of_non_empty_set
    skip
    set1 = CustomSet.new []
    set2 = CustomSet.new [1]
    assert set2.subset? set1
  end

  def test_non_empty_set_is_not_a_subset_of_empty_set
    skip
    set1 = CustomSet.new [1]
    set2 = CustomSet.new []
    refute set2.subset? set1
  end

to

  def test_empty_set_is_a_subset_of_non_empty_set
    skip
    set1 = CustomSet.new []
    set2 = CustomSet.new [1]
    assert set1.subset? set2
  end

  def test_non_empty_set_is_not_a_subset_of_empty_set
    skip
    set1 = CustomSet.new [1]
    set2 = CustomSet.new []
    refute set1.subset? set2
  end

and back again when I get a comment that it is incorrect.

Insti · 2016-06-30T08:15:10Z

The second one looks correct, but does not match the code in the currently committed custom_set_test.rb file.
Do you have a version that has not been merged into this PR?

kotp · 2016-06-30T08:30:20Z

I do not have a version that has not been merged in. Would have to dig through the outdated ones to see. But I am pretty sure I had done this a few times and we keep going back and forth.

kotp · 2016-06-30T09:19:36Z

Oh, "when I get a comment that it is incorrect" may be comments from the tests, not from a human. Perhaps the example.rb is not implemented correctly/backwards.

Insti · 2016-06-30T09:37:31Z

The example.rb implementation is incorrect.

The custom-set exercise is generated using data from x-common repository. fixes #365 fixes #348 references exercism/problem-specifications#257

kotp · 2016-07-01T01:32:59Z

  def member?(datum)
    data.any? { |node| node.datum.eql?(datum) }
  end
+  alias include? member?


Enumerable aliases member? to include? while internally, it uses member. Standard library Set has Ruby method include? which it aliases to member?.

Insti · 2016-07-13T21:17:21Z

I think the most recent change fixed most of the problems we had with this PR.
I vote we merge it and then keep an eye on it to see if anyone reports any problems.

kotp · 2016-07-14T15:02:01Z

Thanks @Cohen-Carlisle and @Insti the goal of at least having it generated is now complete. Now we can focus on any changes that come out, based on common data.

kotp changed the title ~~Generated Custom Set~~ [WIP] Generated Custom Set Jun 22, 2016

kotp force-pushed the 348_Shrink_and_cleanupcustom_set.json_common_232 branch from 116aebb to fcf30af Compare June 22, 2016 18:03

kotp changed the title ~~[WIP] Generated Custom Set~~ Generated Custom Set Jun 22, 2016

kotp added the ready label Jun 22, 2016

Cohen-Carlisle reviewed Jun 23, 2016
View reviewed changes

kotp force-pushed the 348_Shrink_and_cleanupcustom_set.json_common_232 branch 2 times, most recently from c00472e to 305d594 Compare June 23, 2016 06:42

Insti reviewed Jun 23, 2016
View reviewed changes

kotp force-pushed the 348_Shrink_and_cleanupcustom_set.json_common_232 branch from 305d594 to 695ff51 Compare June 23, 2016 08:04

kotp reviewed Jun 23, 2016
View reviewed changes

kotp mentioned this pull request Jun 23, 2016

Updated tests for the Custom Set problem #365

Closed

Cohen-Carlisle reviewed Jun 24, 2016
View reviewed changes

kotp force-pushed the 348_Shrink_and_cleanupcustom_set.json_common_232 branch from 695ff51 to ee5ff9f Compare June 24, 2016 03:13

Cohen-Carlisle reviewed Jun 24, 2016
View reviewed changes

kotp force-pushed the 348_Shrink_and_cleanupcustom_set.json_common_232 branch from ee5ff9f to 0b14fa1 Compare June 26, 2016 08:16

Insti reviewed Jun 27, 2016
View reviewed changes

kotp force-pushed the 348_Shrink_and_cleanupcustom_set.json_common_232 branch from 0b14fa1 to a6b9c93 Compare June 28, 2016 01:02

kotp force-pushed the 348_Shrink_and_cleanupcustom_set.json_common_232 branch from a6b9c93 to db189a6 Compare June 28, 2016 07:26

kotp force-pushed the 348_Shrink_and_cleanupcustom_set.json_common_232 branch from db189a6 to 9b35ab2 Compare June 30, 2016 09:48

Cohen-Carlisle mentioned this pull request Jun 30, 2016

Generated Custom Set II #388

Closed

Generated Custom Set

e90b94f

The custom-set exercise is generated using data from x-common repository. fixes #365 fixes #348 references exercism/problem-specifications#257

kotp force-pushed the 348_Shrink_and_cleanupcustom_set.json_common_232 branch from 9b35ab2 to e90b94f Compare July 1, 2016 01:30

kotp reviewed Jul 1, 2016
View reviewed changes

kotp merged commit 67c4489 into master Jul 14, 2016

kotp deleted the 348_Shrink_and_cleanupcustom_set.json_common_232 branch August 1, 2016 23:12

gchan pushed a commit to gchan/xruby that referenced this pull request Oct 18, 2016

Deprecate counter exercises. Fixes exercism#383 (exercism#407)

cc9a45f

Insti removed the ready label Apr 17, 2017

Uh oh!

Conversation

kotp commented Jun 22, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Cohen-Carlisle commented Jun 24, 2016

Uh oh!

kotp commented Jun 24, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Cohen-Carlisle commented Jun 24, 2016

Uh oh!

Cohen-Carlisle commented Jun 26, 2016

Uh oh!

kotp commented Jun 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Insti commented Jun 27, 2016

Uh oh!

Cohen-Carlisle commented Jun 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Insti commented Jun 27, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Cohen-Carlisle Jun 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Cohen-Carlisle commented Jun 28, 2016

Uh oh!

Cohen-Carlisle commented Jun 28, 2016

Uh oh!

kotp commented Jun 28, 2016

Uh oh!

Insti commented Jun 28, 2016

Uh oh!

kotp commented Jun 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Cohen-Carlisle commented Jun 30, 2016

Uh oh!

kotp commented Jun 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Insti commented Jun 30, 2016

Uh oh!

kotp commented Jun 30, 2016

Uh oh!

kotp commented Jun 30, 2016

Uh oh!

Insti commented Jun 30, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Insti commented Jul 13, 2016

Uh oh!

kotp commented Jul 14, 2016

Uh oh!

kotp commented Jun 27, 2016 •

edited

Loading

Cohen-Carlisle commented Jun 27, 2016 •

edited

Loading

Cohen-Carlisle Jun 27, 2016 •

edited

Loading

kotp commented Jun 28, 2016 •

edited

Loading

kotp commented Jun 30, 2016 •

edited

Loading