Skip to content

Add Functional Programming - Generics as Type Classes #249

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 14 commits into from
Apr 16, 2021

Conversation

jhwgh1968
Copy link
Contributor

I put this under functional languages, since it is really an example of the concept of "type classes" in Rust. If this specific example is more of a design pattern, I would be happy to move it.

If no one beats me to it, I should be able to fix the two FIXMEs at the bottom sometime next week.

@jhwgh1968
Copy link
Contributor Author

Hmm, the errors seem to be caused by broken links. I thought relative links would work in mdbook.

Any guidance on how to fix those?

Copy link
Collaborator

@simonsan simonsan left a comment

Choose a reason for hiding this comment

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

I think it's just that the patterns subdirectory was missing

@simonsan
Copy link
Collaborator

This is related to #100 isn't it? Maybe you can take the links from there, I posted some articles in it, that describe the pattern.

@simonsan simonsan added C-addition Category: Adding new content, something that didn't exist in the repository before A-functional Area: Content about Functional Programming A-pattern Area: Content about Patterns labels Mar 28, 2021
@jhwgh1968 jhwgh1968 force-pushed the state_pattern branch 2 times, most recently from f41f78b to 8a48392 Compare March 28, 2021 15:04
@jhwgh1968
Copy link
Contributor Author

Thanks, @simonsan. I actually missed that PR somehow.

I have added the bottom of the two links you mentioned. Now my question is, how do I break up the link for the hard wrap limit? It is longer than 80 characters by itself.

@simonsan simonsan mentioned this pull request Mar 28, 2021
@simonsan
Copy link
Collaborator

@jhwgh1968
Copy link
Contributor Author

The second link makes sense to me, @simonsan, and seem to cover the type theory in more depth. I also put in a small wording change to be more precise, and helpful if people Google this.

I think it's a solid first version that is ready to merge, unless you have other additions.

@simonsan
Copy link
Collaborator

The second link makes sense to me, @simonsan, and seem to cover the type theory in more depth. I also put in a small wording change to be more precise, and helpful if people Google this.

I think it's a solid first version that is ready to merge, unless you have other additions.

I need to give it a full read still, haven't completely reviewed it due to time constrains and just helped you fix the CI tests. Will take care of it the next two days. Cheers.

simonsan
simonsan previously approved these changes Mar 29, 2021
Copy link
Collaborator

@simonsan simonsan left a comment

Choose a reason for hiding this comment

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

@marcoieni @pickfire Want to give it a read as well?


Here is what that looks like:

```rust,ignore
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would prefer to remove this ignore flag, even if it requires copy pasting from previous code blocks.
What other people think about this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could be nice to have a working example here, indeed. So people can copy and paste it and try it out/play around with it.

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 only did that to avoid repeating all the typing. I can easily put that back in and make it work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, this is turning out to be trickier than I thought. I may have to refactor the example to make it simple enough. That will take me a bit.

Copy link
Collaborator

@simonsan simonsan Apr 1, 2021

Choose a reason for hiding this comment

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

If you want to you can do that in another PR as well (regarding removing the ignore). It would be nice to have a working example that is checked, but not always necessary. Also this is quite complex so I'm fine with just refactoring it at a later stage, maybe even by someone else. @marcoieni thoughts?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's up to jhwgh. If you want you can do it in another PR :)

@marcoieni
Copy link
Collaborator

What's the advantage of using this approach for state machines with respect to the one covered by the rust book ?

@jhwgh1968
Copy link
Contributor Author

The main advantage over the book, @marcoieni, is that this allows for shared data and IMO more readable shared impls.

I will type up that explanation in the PR.


Here is what that looks like:

```rust,ignore
Copy link
Collaborator

@simonsan simonsan Apr 1, 2021

Choose a reason for hiding this comment

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

If you want to you can do that in another PR as well (regarding removing the ignore). It would be nice to have a working example that is checked, but not always necessary. Also this is quite complex so I'm fine with just refactoring it at a later stage, maybe even by someone else. @marcoieni thoughts?

interp.compile_script("print('2 + 2')").unwrap();
interp.exec().unwrap();
}
```
Copy link
Contributor

Choose a reason for hiding this comment

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

Example seemed complicated. If someone like me who had no experience with state machine, this will be hard to understand. Just wondering could there be a simpler example?

Say, empty letter -> written letter -> closed letter?

Comment on lines 147 to 150
struct Interpreter<S: State> {
// same fields for execution, but now add:
state_data: S,
}
Copy link
Contributor

@pickfire pickfire Apr 1, 2021

Choose a reason for hiding this comment

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

Seemed extra to me. Why not just Interpreter -> LoadedInterpreter -> ReadiedInterpreter or something similar since they won't share common stuff?

Or maybe this is just to show <S: State>? Or maybe we can show both methods.


```rust,ignore
fn main() {
let mut interp = Interpreter::<Init>::default().init();
Copy link
Contributor

@pickfire pickfire Apr 1, 2021

Choose a reason for hiding this comment

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

Having this turbofish is the downside of what I mentioned. Not ergonomics and what if user does Interpreter::<Loaded>::default() instead? At least default isn't there in this case which is safe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The default only being in the init state is precisely what I intended, for that exact reason.

I agree it's clunky, but also I think it's reasonable to recomment. If you don't put the turbofish in at all, I think the current stable has a good suggestion to lead you in the right direction.

@jhwgh1968
Copy link
Contributor Author

jhwgh1968 commented Apr 11, 2021

I have a couple more review comments to go, but I am getting close.

Can you specify that with code size we mean the size of the binary?

Done.

Also please, do not force push. It's easier for us to see what changes from review to review.
Everything will be squashed in one commit anyway when we merge this.

On other PRs I saw maintainers doing "merge commits" with master, which usually creates a giant mess in git. I will keep that in mind that you always squash, and not worry about it.

The problem is that mdbook automatically adds fn main() { } around everything because it doesn't exist.

Ah, that makes sense. I will fix it.

@jhwgh1968
Copy link
Contributor Author

jhwgh1968 commented Apr 11, 2021

The main function I added to the compiling example is currently empty. I am planning to put in a simple example of using the types, but I will have to think about it (because construction is something I skipped over).

@marcoieni
Copy link
Collaborator

marcoieni commented Apr 11, 2021

On other PRs I saw maintainers doing "merge commits" with master, which usually creates a giant mess in git. I will keep that in mind that you always squash, and not worry about it.

We set this in December:
image

I have a couple more review comments to go, but I am getting close.

Yes, you are! 💪

jhwgh1968 and others added 2 commits April 11, 2021 19:29
Co-authored-by: Marco Ieni <[email protected]>
Co-authored-by: Marco Ieni <[email protected]>
@simonsan
Copy link
Collaborator

Cool! I'll also read through it again in the next days. <3

marcoieni
marcoieni previously approved these changes Apr 15, 2021
Copy link
Collaborator

@marcoieni marcoieni left a comment

Choose a reason for hiding this comment

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

This is great! I approved it. simonsan, merge this if it looks good to you once you have read it :)

@marcoieni
Copy link
Collaborator

marcoieni commented Apr 15, 2021

The CI is failing for reasons unrelated to this PR:
image

@jhwgh1968
Copy link
Contributor Author

Thanks, @marcoieni! I unfortunately could not think of an easy constructor for main, but I think this example is clear enough as it is.

@simonsan
Copy link
Collaborator

Nice, thank you! :)

@simonsan simonsan merged commit 680509a into rust-unofficial:master Apr 16, 2021
@jhwgh1968 jhwgh1968 deleted the state_pattern branch November 27, 2022 15:12
@jhwgh1968 jhwgh1968 restored the state_pattern branch November 27, 2022 15:13
@jhwgh1968 jhwgh1968 deleted the state_pattern branch November 27, 2022 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-functional Area: Content about Functional Programming A-pattern Area: Content about Patterns C-addition Category: Adding new content, something that didn't exist in the repository before
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants