Skip to content
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

Add module documentation for Mealy #39

Merged

Conversation

eviefp
Copy link
Member

@eviefp eviefp commented Sep 28, 2020

Description of the change
Fixes #35 . It's far from perfect, but perhaps it's a decent start?

Should I update the changelog if I'm not changing any code?


Checklist:

  • Added the change to the changelog's "Unreleased" section with a link to this PR and your username
  • Linked any existing issues or proposals that this pull request should close
  • Updated or added relevant documentation in the README and/or documentation directory
  • Added a test for the contribution (if applicable)

@thomashoneyman
Copy link
Contributor

Thanks! You can update the changelog if you would like, under the "Other improvements" section. Documentation updates are worthy of recognition in the changelog, I think, though it's not necessary if you don't want to.

@JordanMartinez
Copy link
Contributor

I think doc updates should be included, not so much for the recognition, but more because those who didn't understand this library beforehand can be notified to look at it again and see whether it makes sense now.

@eviefp
Copy link
Member Author

eviefp commented Sep 29, 2020

All right, will do so on both PR's.

-- | For example, logging could be used as a sink:
-- | ```purescript
-- | take 10 $ source (pure 1) >>> sink logShow
-- | ```
sink :: forall f a. (Monad f) => (a -> f Unit) -> Sink f a
Copy link
Contributor

Choose a reason for hiding this comment

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

As I look at these, this Monad constraint looks too powerful. Just a thought, but perhaps in a later PR we could relax this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I also noticed that but assumed it was a deliberate decision that I didn't understand...? If it's not, then I'll be happy to fix them.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we ought to, if you’re up for it!

Copy link
Contributor

@thomashoneyman thomashoneyman left a comment

Choose a reason for hiding this comment

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

I have a couple suggestions in here, but overall this is a serious improvement and the comments you've written are concise and clear. Thank you for taking the time to add these. 👍

src/Data/Machine/Mealy.purs Outdated Show resolved Hide resolved
src/Data/Machine/Mealy.purs Outdated Show resolved Hide resolved
@eviefp
Copy link
Member Author

eviefp commented Sep 29, 2020

I think I addressed all review commends. I also added a line in the changelog.

@thomashoneyman
Copy link
Contributor

Excellent! Thanks so much.

@thomashoneyman thomashoneyman merged commit d33d459 into purescript-contrib:main Sep 29, 2020
@eviefp eviefp deleted the feature--module-documentation branch September 30, 2020 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Library doesn't have sufficient module documentation
3 participants