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 a Quick start section in the README file #38

Merged

Conversation

eviefp
Copy link
Member

@eviefp eviefp commented Sep 28, 2020

Description of the change
FIxes #33 . I added a simple (pure) example using Ιdentity over basic operations over integers and strings.

I did not update the changelog since it doesn't affect the code in any way. Should I?


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)

@eviefp eviefp changed the title Feature quick start section Add a Qick start section in the README file Sep 28, 2020
@eviefp eviefp changed the title Add a Qick start section in the README file Add a Quick start section in the README file Sep 28, 2020
@paluh
Copy link
Contributor

paluh commented Sep 29, 2020

@Vladciobanu Thanks a lot for this contribution! This is really cool!

The below is rather a side note which I want to discuss. I wasn't sure where to put these thoughts so I'm writing them here :-)

I wonder if we can add a direct reference to the Mealy definition (copy it from the wikipedia or something) somewhere here and show how it corresponds to the type MealyT f s a. I want to do this because from my perspective it is a quite interesting twist in this implementation that state is not represented directly in the type but we have only input (s) and output (a) and context (f) params.
I propose to add some examples which show how to carry state using just plain closure but also by usingState. This snippet comes from Haskell machines lib. It clearly separates state from the input:

>>> let countingMealy = unfoldMealy (\i x -> ((i, x), i + 1)) 0
>>> run (auto countingMealy <~ source "word")
[(0,'w'),(1,'o'),(2,'r'),(3,'d')]

and here we have an implementation of unfoldMealy which should probably be added to the lib (?):

-- | A 'Mealy' machine modeled with explicit state.
unfoldMealy :: (s -> a -> (b, s)) -> s -> Mealy a b
unfoldMealy f = go where
  go s = Mealy $ \a -> case f s a of
    (b, t) -> (b, go t)

Of course the above snippets are related to the simpler representation of Mealy from the original lib:

newtype Mealy a b = Mealy { runMealy :: a -> (b, Mealy a b) }

In our case we can probably use this but also just provide an example using StateT / State. I can add this in a separate PR. What do you think?

@thomashoneyman
Copy link
Contributor

I wonder if we can add a direct reference to the Mealy definition (copy it from the wikipedia or something) somewhere here and show how it corresponds to the type MealyT f s a. I want to do this because from my perspective it is a quite interesting twist in this implementation that state is not represented directly in the type but we have only input (s) and output (a) and context (f) params.

I think this would be a nice addition.

I propose to add some examples which show how to carry state using just plain closure but also by using State.

I think this is also a nice idea. We should endeavor to keep the quick start small (just enough to give you the context and most basic examples to get you started with the library), so as long as we can capture this in a quick example or two I think it makes a great fit.

There's also the docs/ folder for any longer writeups or treatments of this! For example, that might be a good place for a longer explanation of what Mealy machines are, why they're useful, and how they correspond to MealyT.

and here we have an implementation of unfoldMealy which should probably be added to the lib (?):

Also a nice idea.

@Vladciobanu @paluh I don't think these additions need to hold up this PR, and could be done in a subsequent one if you'd prefer to handle them @paluh -- unless you'd rather do it, @Vladciobanu. We can iterate on these docs sections :)

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Comment on lines +56 to +61
haltOn0 :: Int -> Step Identity Int Int
haltOn0 0 = Halt
haltOn0 n = Emit n $ pureMealy haltOn0

scale :: Int -> Step Identity Int Int
scale n = Emit (n `div` 2) $ pureMealy scale
Copy link
Contributor

@thomashoneyman thomashoneyman Sep 29, 2020

Choose a reason for hiding this comment

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

For parity with the earlier example, should these two be merged?

    scale :: Int -> Step Identity Int Int
    scale n
      | n == 0 = Halt
      | otherwise = Emit (n `div` 2) $ pureMealy scale

as this is introduced as "the same machine as before".

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right! I changed the examples a bit, using guard in the monadic version. Does this look better?

Copy link
Contributor

Choose a reason for hiding this comment

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

If you are comfortable switching to two spaces instead of four in the example (as is done in the do example above) I'd appreciate that, but otherwise 👍!

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm going to leave this unmerged to give you a moment to respond to this, but at that point I will go ahead and merge.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@paluh
Copy link
Contributor

paluh commented Sep 29, 2020

@thomashoneyman Should I move the above comment to some separate issue like "Document correspondence of MealyT to its abstract model" or something?

@thomashoneyman
Copy link
Contributor

If it's something to go in the quick start then I'd like to let @Vladciobanu have a chance to see whether and if he'd like to work that in to his existing content here, but if it's for the docs directory then feel free to go ahead with the issue and I can assign it to you!

@eviefp
Copy link
Member Author

eviefp commented Sep 29, 2020

I think the wikipedia definition would be a great addition to the Mealy module documentation rather than the Quick start section, or even the README.md file. I say this mostly because I assume we might add more machines to this library. I'm happy to take this on in a subsequent PR!

I also think that more in-depth examples belong in either tests or docs, again, in subsequent PR's.

I also added imports to the examples. Mostly because I decided to use guard and that's not in Prelude, so I figured it's going to be easier to run the examples.

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.

Thanks for adding this, @Vladciobanu. I think that we should create followup issues or get right into the PRs for the changes that you and @paluh mentioned in this thread. We may need to briefly chat about who is going to do what just to make sure that there isn't any wasted / overlapping work. We could do that in this PR thread or in a chat on Discourse -- whatever y'all prefer :)

@thomashoneyman
Copy link
Contributor

Also, once this is merged I will release a new version of the library and push the docs to Pursuit. Then we'll have a nice landing page for -machines and some module docs!

@eviefp
Copy link
Member Author

eviefp commented Sep 30, 2020

I propose we create a few issues:

  • Explain Mealy in Module Docs: Add wikipedia definition and explain how it connects to the MealyT type.
  • Rename Type Variables: Rename MealyT f s a to MealyT effect input output or MealyT eff i o

I also propose we add the details that @paluh wrote in his original comment here to the docs issue, as an example of something that would be useful to be added there.

@thomashoneyman
Copy link
Contributor

Sounds good to me! Let’s start there.

@thomashoneyman thomashoneyman merged commit d558e0e into purescript-contrib:main Sep 30, 2020
@eviefp eviefp deleted the feature--quick-start-section 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.

Repository doesn't have a quick start section in the README
3 participants