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

Step 1 toward Prism documentation: a "how to" module for Prisms. #88

Merged
merged 13 commits into from May 7, 2018

Conversation

Projects
None yet
4 participants
@marick
Copy link
Collaborator

marick commented Apr 28, 2018

I've created an examples directory and populated it with a file that shows how to use prisms on sum types.

Once this is approved, I'll add text and examples to the Prism API documentation.

"purescript-prelude": "^3.3.0",
"purescript-console": "^3.0.0",
"purescript-profunctor-lenses": "^3.8.0",
"purescript-record-show": "^0.4.0",

This comment has been minimized.

@paf31

paf31 Apr 30, 2018

Member

I would like to avoid adding non-contrib libraries as dependencies if we can help it.

This comment has been minimized.

@paf31

paf31 Apr 30, 2018

Member

(because it makes updating things in order more difficult)

This comment has been minimized.

@marick

marick May 1, 2018

Collaborator

Is this an issue for the next release?

If not, I suggest we err on the side of clarity.

This comment has been minimized.

@marick

marick May 6, 2018

Collaborator

I was about to fix the use of purescript-record-show, but it seems to me purescript-color may also be non-contrib. https://pursuit.purescript.org/packages/purescript-colors/4.3.0/docs/Color Is it?

Removing Color would be more work than I'm willing to do. Plus there's value in more realistic examples.

This comment has been minimized.

@dwhitney

dwhitney May 7, 2018

came here to +1 this. I see where @paf31 is coming from, but.... Lenses are so simple and helpful, yet like @marick has said on Twitter - it took me so long to wrap my head around them and these simple examples would have made the mental leap almost instant. At the moment, I'm somewhat reluctant to use lenses on my project for fear that others will come on and be confused and unproductive.

This comment has been minimized.

@paf31

paf31 May 7, 2018

Member

The reason is that is makes updating libraries unnecessarily difficult for maintainers, since most contrib libraries fall on the core library maintainers to update anyway. colors is actually a counterexample since it was added as a dependency of drawing accidentally already, so I have no issue with adding a (dev-)dependency on colors.

In any case, these should be dev-dependencies, not actual dependencies, since they're only used in documentation. In fact, I don't know if they need to be dependencies at all. The documentation can just instruct the user to bower install whatever it needs.

This comment has been minimized.

@marick

marick May 7, 2018

Collaborator

I'll move those to the examples README if required. Before then, a question: did you notice that the bower.json file isn't the top-level one, but one within examples?

So, putting:

bower i purescript-color
bower i purescript-show-record

... into examples/README.md is really the same as putting them in bower.json, except it's even less likely that a maintainer will notice the incompatibility. (In either case, users will.)

This comment has been minimized.

@paf31

paf31 May 7, 2018

Member

No I didn't notice, thanks for pointing that out. In that case, I think this is fine.

This comment has been minimized.

@paf31

paf31 May 7, 2018

Member

It does unfortunately mean that they might not get updated at the same time as the source files during major version changes, while we wait for those libraries to get updated.

This comment has been minimized.

@garyb

garyb May 7, 2018

Member

I do this kind of thing (or have done in the past at least) with the examples for Halogen. I think it's a good compromise between making useful examples and not being constrained by other libraries when wanting to make a new release.

@marick marick merged commit 02b1693 into purescript-contrib:master May 7, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@paf31

This comment has been minimized.

Copy link
Member

paf31 commented May 7, 2018

Please squash merge, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment