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

Start Pasers Combinators lesson #44

Open
wants to merge 17 commits into
base: development
Choose a base branch
from

Conversation

blackheaven
Copy link
Contributor

Submitter checklist

@blackheaven blackheaven added the documentation Improvements or additions to documentation label Dec 11, 2021
@blackheaven blackheaven marked this pull request as ready for review January 12, 2022 21:33
@TrueBoxGuy
Copy link
Contributor

Thank you very much for your contributions: I'd be happy to review this PR 🙂.

@blackheaven
Copy link
Contributor Author

Is there anything I can do to make it merged?

@Kleidukos
Copy link
Contributor

I can take a look early next week :)

@blackheaven
Copy link
Contributor Author

I'll be grateful, I take any feedback.

@TrueBoxGuy
Copy link
Contributor

I am very sorry for having you wait so long.

I was slightly busier than expected and I pushed your PR further back in my mind than I should have.

I mainly ensure contribution to the style guide on the parts of the PR where relevant (my review may seem a little repetitive).

Copy link
Contributor

@TrueBoxGuy TrueBoxGuy left a comment

Choose a reason for hiding this comment

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

I find your introduction quite nice. As you said, it definitely is beneficial to teach readers about parsers because they are an extremely practical application of combinators.

In terms of readability, I think it might be useful if those here talked about the order in which your articles here should be read, and the assumed familiarity with Haskell.

(My review is rather incomplete: I started it today so it would be easier to pick up again during the week.)

en/lessons/parsers/introduction.md Outdated Show resolved Hide resolved
en/lessons/parsers/introduction.md Outdated Show resolved Hide resolved
en/lessons/parsers/introduction.md Outdated Show resolved Hide resolved
en/lessons/parsers/introduction.md Outdated Show resolved Hide resolved
en/lessons/parsers/introduction.md Outdated Show resolved Hide resolved

As you can see, relying on `Monad`, allows us to focus on the structure on the input, instead of the structure on the Parser.

then, we can define our main parser, composition:
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 the comma here gets in the way of clarity.

en/lessons/parsers/introduction.md Outdated Show resolved Hide resolved
en/lessons/parsers/introduction.md Outdated Show resolved Hide resolved
@blackheaven
Copy link
Contributor Author

No worries, I may be involved in the security track, and I'll try to be focused on that, sorry for the sudden wake up.

I'll handle your feedback later today, thanks.

@TrueBoxGuy
Copy link
Contributor

Was the recent commit made as the result of some external review (e.g. conducted on Slack) or just on your own?

I think it's extremely useful to know how you desire your articles to be used and it is useful for those learning Haskell to see how library code is written. It's been slightly difficult for me to review any of the other more technical articles, but it has been on my mind this time. Given my main role is reviewing grammar and I don't have much knowledge of parsers, it's quite hard to review an article that's mainly code. However, what I could perhaps do, in absence of this knowledge, is learn about one of the simpler libraries, and then give some advice about any changes that could be made to articles (structurally / gramatically) to have them better serve the purpose you want, to be applied to the other articles in general. Would that work?

I'm also able to call for however long if you want over the next week, but I'd rather learn a bit about the libraries first to make use of a call.

@blackheaven
Copy link
Contributor Author

You're right, I had a quick chat with someone (I'm not sure they want to be mentioned) to clarify the targeted audience.

You're also right about the structure of the contribution (mostly code), which might not be useful.

I'll have another look at it, so it will be more helpful for beginners, going more in depth of each piece of code.

@TrueBoxGuy
Copy link
Contributor

I had a small look over the ReadP markdown file and I think the following would be useful in achieving the goal of showing beginners some common Haskell idioms:

  • An initial mention of how ReadP and P are connected before introducing both of them. You do mention that P represents the current instruction, but perhaps the design pattern used in the module should be emphasised more. The design pattern, where P essentially represents a parser expression and ReadP is a continuation used to build a parser expression, is fairly common (especially in language interpreters).

    Similarly, although one can think through the underlying code of the P instances, some readers may be wondering why they exist (or alternatively, why ReadP exists at all). This may be substantially difficult to explain (and truly understanding code requires understanding history and the intent of the authors): the parser library can be minimally defined, with some modification, by: P (with no instances defined for it), (+++), (<++); ReadP and its instances, along with its operators and derived operators. I believe the other instances exist because it is idiomatic to implement them if possible, and some code is necessitated by Alternative being a subclass of Applicative. One must also consider that many of these typeclasses are rather recent.

    Explaining the entirety of why the ReadP code is written as it is is quite complicated, but highlighting some of the idioms commonly seen in code (such as defining all possible instances) may be useful for beginners.

    It would also be useful to talk about the value of these idioms (it is harmful to apply idioms without knowing why they exist).

  • Removing comments and whitespace in code and then explaining the code and comments yourself: some of the code comments are rather short and aren't meant for beginners. For example, to a beginner, it may not immediately be obvious what the Get, Look, Fail, Result, and Final operations mean. Similarly, doing this would be a good opportunity to explain the idioms used in designing (<|>) efficiently, and the idiomatic way in which evaluators (such as run) are written.

  • Showing some of the derived operators, as they're useful examples of how parser combinators are used.

  • Making more concrete comparisons in the conclusion. It isn't immediately obvious how simplicity corresponds to inefficiency. I think more concrete points can be made by comparing ReadP to other libraries (such as restating in the conclusion how ReadP operates on strings and returns all possible parses).

I think the order in which you introduce ReadP's constructs is quite good: the main thing that's needed is a little more explicit explanation of the idioms for beginners.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants