Skip to content

Add examples #36

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
Dec 6, 2020
Merged

Add examples #36

merged 3 commits into from
Dec 6, 2020

Conversation

milesfrain
Copy link
Contributor

No description provided.

Copy link
Contributor

@hdgarrood hdgarrood left a comment

Choose a reason for hiding this comment

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

Looks great - thanks!

-- | nonEmptyArray :: NonEmpty Array Int
-- | nonEmptyArray = NonEmpty 1 [2,3]
-- |
-- | import Data.List(List(..), (:))
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be preferable to move the import declarations to the top, as you aren't allowed to have any import declarations after the first non-import declaration in a module.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was trying to match the style of using the repl, and keeping things logically clustered. I guess it's a question of whether this slight addition of clarity/organization outweighs the confusion if someone pastes this into a .purs file. I think the error message is pretty clear though in that case.

Luckily this won't be a concern with doctest (repl style).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this a blocker? I'm fine with changing it, but it doesn't seem too consequential either way, so I'll spare the effort if it's unnecessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, it’s just that changes to the core libraries should generally obtain two core team approvals, see https://github.com/purescript/governance

Comment on lines 115 to 116
-- | For example:
-- |
Copy link
Member

Choose a reason for hiding this comment

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

I think we typically don't include "For example:" just to save on the editor tooltips so the example is closer to hand. We could go straight to the example:

-- | Get the 'first' element of a non-empty container
-- |
-- | ```purescript
-- | head (1 :| [2, 3]) == 1
-- | ```

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was following precedent set by existing docs in this file. Will remove this phrasing from all of them.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I didn't realize. Thank you for removing that.

-- | Apply a function that takes the `first` element and remaining elements
-- | as arguments to a non-empty container.
-- |
-- | For example, return the remaining elements multiplied by the first element:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Keeping this additional example clarification.

@thomashoneyman thomashoneyman merged commit 184e6a6 into purescript:master Dec 6, 2020
@milesfrain milesfrain deleted the examples branch December 6, 2020 06:55
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.

3 participants