Docs: Set / SortedList added details regarding equality checks. #5131

miriamvoth · 2020-08-09T14:12:41Z

Purpose and Motivation

Added information to the help files Set.schelp and SortedList.schelp, if a function checks for equality or identity.
Connected to #4573 (not fully fixing the issue yet)

Types of changes

Documentation

To-do list

Code is tested
All tests are passing
Updated documentation
This PR is ready for review

elgiano

Thank you @miriamvoth for wanting to take care of this!

Looks ok to me! Proposed a few changes.
Just to be sure: is the purpose of this to be really explicit about when functions check equality vs. identity? It looks like, across the class library, most functions check for equality unless they explicitly specify that they check for identity. The only counterexamples I've found are indexOf checking for identity VS. indexOfEqual checking for equality, and the same for includes VS includesEqual.

I think a lot of confusion comes from different Collections having different includes, some checking for equality and some for identity. So, together with documenting every method as we're doing here, we should make sure to document those different "includes", for example Set:includes is not documented.

elgiano

had lost all my code-comments in my previous comment. Here they are again

elgiano · 2020-08-09T19:33:12Z

HelpSource/Classes/SortedList.schelp

@@ -5,7 +5,7 @@ categories::Collections>Ordered
 CLASSMETHODS::

 method::new
-Creates a SortedList with the initial capacity given by strong::size:: and a comparison strong::function::.
+Creates a SortedList with the initial capacity given by strong::size:: and a comparison strong::function::. The SortedList can contain equal elements more than once.


If we really want to state that SortedList can contain duplicates (maybe superfluous IMO since this is what most Collections do), then we should say "equal or identical"

elgiano · 2020-08-09T19:36:00Z

HelpSource/Classes/Set.schelp

@@ -74,6 +75,7 @@ a -- b // shorter syntax

 method::isSubsetOf
 Returns true if all elements of this are also elements of strong::that::.
+Checks for equal elements regardless of the order inside the set.


Set is unordered. While I think it doesn't hurt to state that again, maybe it should be in a form that supports a more solid knowledge of this important property?
Example:
"Checks for equality (not identity). Since Set is an unordered collection, order doesn't matter in this comparison"

Sounds good. I just took your sentence.

elgiano · 2020-08-09T19:38:25Z

HelpSource/Classes/Set.schelp

@@ -41,23 +41,23 @@ Returns the object at the internal strong::index::. This index is not determinis
 subsection::Set specific operations

 method::sect, &
-Return the set theoretical intersection of this and strong::that::.
+Return the set theoretical intersection of this and strong::that::. The function will search for strong::equal:: objects occurring in both sets and return a new set containing those.


for ::sect and ::union I would personally stick to the "equality (not identity)" style you use everywhere else. Also "equal (not identical)" would be fine for me.

miriamvoth · 2020-08-10T19:21:38Z

Hi @elgiano, thanks for your feedback.
I do not know supercollider too well yet, so I do not have an overview how many functions might behave differently from what a user expects. As I understood the issue #4573 it would be helpful to be explicit about the check in all of the functions which do some kind of matching.
If you think it would improve the documentation to do so, I will proceed with it.
If you think it is actually unnecessary, I would rather concentrate on the few functions, that seem ambiguous / unclear to me.

elgiano · 2020-08-11T10:34:19Z

Hi @miriamvoth! I want to make sure you know that I absolutely don't intend to take your work down in any way! I agree it would be better to be explicit about equality vs. identity than to be implicit, and it's great that you are willing to take care of that!

I think it would be great to:

document all ".includes" / ".indexOf" methods: I think the main source of confusion comes from different collection having different .includes or .indexOf methods, so it would be very important to document those, especially where they are completely undocumented (such as for Set)
Then let's aim to annotate every function that derives from those (as you already started doing), and any other method where you feel confusion can rise. From my experience, I think most often people don't read docs top to bottom, but just check for a specific function, and they would benefit from a clarification being just there

I would move on with this as follows:

check my (and eventually anyone else's :) ) comments. My code comments are there to help you with wording consistency and to highlight fundamental features of specific collections, where I felt it was appropriate. Please consider them, and let me know if you agree :)
let's resolve those conversations
move on to other classes as you feel/can
we'll have more discussion/comments about those, and repeat this process until we are done

EDIT: and of course if this whole process feels overwhelming or in any way too much work for you, absolutely feel free to do only as much work as you feel good about doing! Contribution is totally voluntary and every contribution is really appreciated! :)

Finally, if we will come up with a summary of differences among Collections, I think we could put in a paragraph about it.. maybe in Collection's HelpFile Description section?

And in case this could help you, by writing .includes in SCIDE and pressing ctrl+i (or cmd+i on Mac), you can see a list of all the different implementations of includes from different Classes. Same thing ofc for indexOf :)

mossheim · 2020-08-11T11:34:25Z

@elgiano just for context, i specifically told @miriamvoth that for this initial PR they should only document a few items, so that we can get a pattern that we like, before going on to document more. that first step is the hardest in my opinion. :)

miriamvoth · 2020-08-11T18:19:26Z

@elgiano thanks for your detailed answer :)

I included your suggestions in my new commit and went for a more unified wording with the equal (not identical).
For union and sect I would like to keep an explanatory sentence, because maybe not every non-programmer knows the technical meaning of those words.

For Set I also documented the functions .includes() and .findMatch() now and I will keep an eye on .includes and .indexOf in the next classes to come.

elgiano · 2020-08-18T22:11:46Z

@miriamvoth thanks, and sorry for my delay!
This being the first of more changes alike, I feel an extra responsibility and will expose all my doubts. Now that I see "Checks for equal (not identical)" everywhere, I feel this notation suggests that those checks accept equal elements while rejecting identical elements. What do you think? Am I being too pedantic?

If you agree with me, this could be a possible replacement:
"Matches are checked by equality (not by identity)"

What do you think?

mossheim · 2020-08-18T22:49:11Z

Now that I see "Checks for equal (not identical)" everywhere, I feel this notation suggests that those checks accept equal elements while rejecting identical elements. What do you think? Am I being too pedantic?

If you agree with me, this could be a possible replacement:
"Matches are checked by equality (not by identity)"

What do you think?

fwiw, i think your suggested replacement is clearer. "check for equality" is more common than "check by equality", i think. for example:

https://dotnetcodr.com/2017/03/18/equality-checking-in-f/
https://stackoverflow.com/questions/9977236/how-to-check-for-value-equality
https://stackoverflow.com/questions/3191969/5-ways-for-equality-check-in-net-why-and-which-to-use

and all elements, not just matches, are checked. so i'd amend that to: "Elements are checked for equality (not for identity)"

miriamvoth · 2020-08-23T08:22:51Z

Yeah, I was also wondering, if the sentence might be misunderstood without context. Especially since I am not a native speaker, it is hard for me to tell. I will change the sentence accordingly.

…ecks

mossheim

thanks! sorry for the long delay, everything looks good

mossheim added the comp: help schelp documentation label Aug 9, 2020

mossheim requested review from elgiano, jrsurge and patrickdupuis August 9, 2020 15:33

elgiano reviewed Aug 9, 2020

View reviewed changes

miriamvoth added 2 commits August 23, 2020 13:21

Docs: Set / SortedList added details regarding equality checks.

f105fd7

Docs: Set / SortedList: added documentation for .includes / .findMatch

358f85f

miriamvoth force-pushed the topic/help-equality-checks branch from b1531d8 to 358f85f Compare August 23, 2020 11:24

Docs: Set / SortedList: clarified documentation regarding equality ch…

ec7bd26

…ecks

mossheim approved these changes Oct 11, 2020

View reviewed changes

mossheim merged commit 3c0762d into supercollider:develop Oct 11, 2020

This was referenced Oct 24, 2020

[3.11] cherry-pick documentation PRs for 3.11.2 #5214

Merged

[3.11] Cherry-pick several functional PRs for 3.11.2 #5215

Merged

miriamvoth deleted the topic/help-equality-checks branch April 17, 2021 17:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Docs: Set / SortedList added details regarding equality checks. #5131

Docs: Set / SortedList added details regarding equality checks. #5131

miriamvoth commented Aug 9, 2020

elgiano left a comment •

edited

Loading

elgiano left a comment

elgiano Aug 9, 2020

elgiano Aug 9, 2020

miriamvoth Aug 11, 2020

elgiano Aug 9, 2020

miriamvoth commented Aug 10, 2020

elgiano commented Aug 11, 2020 •

edited

Loading

mossheim commented Aug 11, 2020

miriamvoth commented Aug 11, 2020

elgiano commented Aug 18, 2020

mossheim commented Aug 18, 2020

miriamvoth commented Aug 23, 2020

mossheim left a comment

Docs: Set / SortedList added details regarding equality checks. #5131

Docs: Set / SortedList added details regarding equality checks. #5131

Conversation

miriamvoth commented Aug 9, 2020

Purpose and Motivation

Types of changes

To-do list

elgiano left a comment • edited Loading

Choose a reason for hiding this comment

elgiano left a comment

Choose a reason for hiding this comment

elgiano Aug 9, 2020

Choose a reason for hiding this comment

elgiano Aug 9, 2020

Choose a reason for hiding this comment

miriamvoth Aug 11, 2020

Choose a reason for hiding this comment

elgiano Aug 9, 2020

Choose a reason for hiding this comment

miriamvoth commented Aug 10, 2020

elgiano commented Aug 11, 2020 • edited Loading

mossheim commented Aug 11, 2020

miriamvoth commented Aug 11, 2020

elgiano commented Aug 18, 2020

mossheim commented Aug 18, 2020

miriamvoth commented Aug 23, 2020

mossheim left a comment

Choose a reason for hiding this comment

elgiano left a comment •

edited

Loading

elgiano commented Aug 11, 2020 •

edited

Loading