added examples to array module documentation #121

csicar · 2017-12-15T01:01:01Z

this commit adds simple examples to the documentation comments for most functions defined in Data.Array

fehrenbach · 2017-12-15T09:04:17Z

src/Data/Array.purs

 -- | Running time: `O(n)` where `n` is the length of the array
 tail :: forall a. Array a -> Maybe (Array a)
 tail = uncons' (const Nothing) (\_ xs -> Just xs)

 -- | Get all but the last element of an array, creating a new array, or
 -- | `Nothing` if the array is empty.
 -- |
+-- | ```purescript
+-- | init [1, 2, 3, 4] = Just [2, 3, 4]


I think you mean [1, 2, 3] :)

fehrenbach · 2017-12-15T10:25:55Z

src/Data/Array.purs

@@ -571,6 +842,11 @@ group' = group <<< sort

 -- | Group equal, consecutive elements of an array into arrays, using the
 -- | specified equivalence relation to detemine equality.
+-- |
+-- | ```purescript
+-- | groupBy (\a b → odd a && odd b) [1, 1, 2, 2, 3, 3] = [[1, 1], 2, 2, [3, 3]]


For me this prints [(NonEmpty 1 [1]),(NonEmpty 2 []),(NonEmpty 2 []),(NonEmpty 3 [3])] which is not too pretty, but the 2s should be in singleton arrays at least: [[1, 1], [2], [2], [3, 3]]

I used the array-notation, because the example for group also used that notation

Ah, I see. @garyb looks like you made the switch to NonEmpty (in 738f1b4). Did you intentionally not change the examples for group and group'?

No, it wasn't intentional :)

There is something nice about not showing the NonEmpty stuff in the example, as it does make it a bit clearer to see what is going on, but it's inaccurate obviously so I'm not sure which is better really. I guess we do show the exact output usually, so it probably should be in there. I don't know.

hdgarrood

Looks great so far, thanks! I'm fairly confident the best approach for the groupBy example is to display it exactly as it would appear in the repl, to avoid any confusion. If people want to use groupBy they need to understand the NonEmpty type anyway.

hdgarrood · 2017-12-15T13:30:51Z

src/Data/Array.purs

 catMaybes :: forall a. Array (Maybe a) -> Array a
 catMaybes = mapMaybe id

 -- | Apply a function to each element in an array, supplying a generated
 -- | zero-based index integer along with the element, creating an array
 -- | with the new elements.
+-- |
+-- | ```purescript
+-- | mapWithIndex (\index element -> (show index) <> element) ["Hello", "World"] = ["0Hello", "1World"]


You have some redundant parens here

hdgarrood · 2017-12-15T13:40:37Z

src/Data/Array.purs

+-- |
+-- | ```purescript
+-- | unsafePartial $ unsafeIndex ["a", "b", "c"] 1 = "b"
+-- | unsafeIndex ["a", "b", "c"] 10 -- runtime error


If I remember correctly, this won't produce a runtime error. Instead, the result will be undefined - of course, this is likely to cause a runtime error when the result is used, but it could also just cause the program to behave incorrectly but continue running. Would you mind checking this and fixing the example if necessary?

Also, you'll need an unsafePartial here too, presumably?

csicar · 2017-12-15T16:34:04Z

the problem with group etc. could be solved like this:

group [1,1,2,2,1] == [(NonEmpty 1 [1]),(NonEmpty 2 [2]),(NonEmpty 1 [])]
map (fromFoldable) $ group [1,1,2,2,1] == [[1, 1], [2, 2], [1]]

that version would give the induition of the array-notation, while also being correct

hdgarrood · 2017-12-15T16:37:44Z

I think that's a little verbose. I'd rather direct people to the NonEmpty documentation if they don't understand what NonEmpty means. (Of course, Pursuit already does this, because the NonEmpty type in the docs is linked.)

hdgarrood · 2017-12-15T16:41:00Z

Oh also, the parens in eg [(NonEmpty 2 [2])] are redundant; the reason they appear in the REPL currently is just a shortcoming with the Show class (which will be fixed eventually). I think it would be better not to include them in these examples.

csicar · 2017-12-15T16:47:10Z

I also tried to find examples for some and many, but could not understand them. Any ideas for simple example for those functions?

hdgarrood · 2017-12-15T16:53:51Z

This PR is getting quite large, so I'd like to avoid it getting any larger. You can always send more later :) I'll have a think about some and many and get back to you. For now I'd like to just look over this one more time and then merge, as I think we've addressed everything raised now.

csicar · 2017-12-15T16:55:54Z

sounds good 👍

hdgarrood · 2017-12-15T17:10:22Z

src/Data/Array.purs

@@ -324,6 +429,12 @@ foreign import findIndexImpl
  -> Maybe Int

 -- | Find the last index for which a predicate holds.
+-- |
+-- | ```purescript
+-- | findLastIndex (_ > 0) [-10, -4, 2, 3] = Just 3


I think it might be slightly better to use a type other than Int for the elements of the array here, actually, just to avoid potential confusion from people thinking findLastIndex might return an element of the array vs an index into it.

hdgarrood · 2017-12-15T17:13:33Z

src/Data/Array.purs

 -- | ```
 group' :: forall a. Ord a => Array a -> Array (NonEmpty Array a)
 group' = group <<< sort

 -- | Group equal, consecutive elements of an array into arrays, using the
 -- | specified equivalence relation to detemine equality.
+-- |
+-- | ```purescript
+-- | groupBy (\a b -> odd a && odd b) [1, 3, 2, 4, 3, 3] = [NonEmpty 1 [3], NonEmpty 2 [] , NonEmpty 4 [], NonEmpty 3 [3]]


Can you please break this up over multiple lines? I don't think this will look great rendered as HTML. (You can check with pulp docs -- --format html)

hdgarrood · 2017-12-15T17:16:14Z

src/Data/Array.purs

+-- |
+-- | ```purescript
+-- | unsafePartial $ unsafeIndex ["a", "b", "c"] 1 = "b"
+-- | unsafePartial $ unsafeIndex ["a", "b", "c"] 10 -- runtime error


I think this comment is still not quite correct.

I checked again in psci.
Result of evaluating:

> unsafePartial $ unsafeIndex ["a", "b", "c"] 1 "b" > unsafePartial $ unsafeIndex ["a", "b", "c"] 10 /home/.../.psci_modules/node_modules/Data.Show/foreign.js:30 var l = s.length; ^ TypeError: Cannot read property 'length' of undefined at exports.showStringImpl (/home/.../.psci_modules/node_modules/Data.Show/foreign.js:30:13) at /home/.../node_modules/Control.Monad.Eff.Console/index.js:19:53 at Object.<anonymous> (/home.../.psci_modules/node_modules/$PSCI/index.js:19:84) at Module._compile (module.js:641:30) at Object.Module._extensions..js (module.js:652:10) at Module.load (module.js:560:32) at tryModuleLoad (module.js:503:12) at Function.Module._load (module.js:495:3) at Module.require (module.js:585:17) at require (internal/module.js:11:18)

this is with version:

$ pulp --version Pulp version 12.0.1 purs version 0.11.7 using /usr/local/bin/purs

Right, but the error message indicates that the error comes from attempting to Show it - we might not necessarily do that in other situations.

I'll try to come up with a good example

Oh yes, I see. Did not think of that..

For example:

module Main where import Prelude import Data.Array as Array import Partial.Unsafe (unsafePartial) import Control.Monad.Eff (Eff) import Control.Monad.Eff.Console (CONSOLE, log) main :: forall e. Eff (console :: CONSOLE | e) Unit main = do let oops = unsafePartial (Array.unsafeIndex [true] 1) log $ if oops then "it was true" else "it was false"

Runs successfully and prints "it was false", because undefined is falsy.

Since this is a little subtle, maybe it would be best to remove the out-of-range example, and instead explain what happens if you try to unsafeIndex with an out-of-range index in prose. How about:

Using unsafeIndex with an out-of-range index will not immediately raise a runtime error. Instead, the result will be undefined. Most attempts to subsequently use the result will cause a runtime error, of course, but this is not guaranteed, and is dependent on the backend; some programs will continue to run as if nothing is wrong. For example, in the JavaScript backend, the expression unsafePartial (unsafeIndex [true] 1) has type Boolean; since this expression evaluates to undefined, attempting to use it in an if statement will cause the else branch to be taken.

hdgarrood · 2017-12-16T00:30:36Z

src/Data/Array.purs

@@ -560,7 +566,12 @@ foreign import concat :: forall a. Array (Array a) -> Array a
 -- | into a single, new array.
 -- |
 -- | ```purescript
-- | concatMap (split $ Pattern " ") ["Hello World", "other thing"] = ["Hello", "World", "other", "thing"]
+-- | concatMap (split $ Pattern " ") ["Hello World", "other thing"]
+-- | ```


Sorry to nitpick but I think I prefer the style you used in e.g. alterAt, where it's still all contained in a single code block.

hdgarrood · 2017-12-16T01:03:09Z

Fantastic, thank you very much for this!

csicar · 2017-12-16T01:06:53Z

thanks for merging!

added examples for array module documentation

a1d3e09

fehrenbach reviewed Dec 15, 2017

View reviewed changes

csicar added 2 commits December 15, 2017 11:56

corrected examples

7600000

use ascii -> and tested examples

e250a1d

hdgarrood reviewed Dec 15, 2017

View reviewed changes

csicar added 2 commits December 15, 2017 17:21

removed redundant parens

df33495

corrected example for unsafeIndex

a05c088

use actual result for group* functions

34d44a5

removed redundant parens in group* docs

66bf00a

hdgarrood reviewed Dec 15, 2017

View reviewed changes

added linebreaks; use better example for findIndex, findLastIndex

61e1431

hdgarrood reviewed Dec 16, 2017

View reviewed changes

csicar added 2 commits December 16, 2017 01:55

use single code block in examples

ba090d4

better explanation for unsafeIndex out-of-bounds access

4c34ea2

hdgarrood merged commit 0ffc076 into purescript:master Dec 16, 2017

csicar deleted the array-docs-examples branch December 16, 2017 01:07

added examples to array module documentation #121

added examples to array module documentation #121

Uh oh!

Conversation

csicar commented Dec 15, 2017

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!

hdgarrood left a comment

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!

csicar commented Dec 15, 2017

Uh oh!

hdgarrood commented Dec 15, 2017

Uh oh!

hdgarrood commented Dec 15, 2017

Uh oh!

csicar commented Dec 15, 2017

Uh oh!

hdgarrood commented Dec 15, 2017

Uh oh!

csicar commented Dec 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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!

hdgarrood commented Dec 16, 2017

Uh oh!

csicar commented Dec 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

csicar commented Dec 15, 2017 •

edited

Loading

csicar commented Dec 16, 2017 •

edited

Loading