Pull requests

#4 Declined
Repository
blamario/picoparsec picoparsec
Branch
default
Repository
bos/attoparsec attoparsec
Branch
default

Pull request for a new module: Data.Attoparsec.Monoid

Author
  1. Mario Blažević
Reviewers
Description

I've synced my fork with the latest updates on attoparsec, so I'm hereby updating the pull request as well.

If you have no intention of accepting the request, please do tell me so. In that case I'll just publish the fork on Hackage.

  • Learn about pull requests

Comments (7)

  1. Bryan O'Sullivan repo owner

    This is rather a difficult pull request to review. There's a combination of copy-and-paste duplication of existing code (yuck!) and new concepts that are not explained. In fact, let me back up: I don't even understand the motivation for this module in the first place, so that needs explaining.

    The commits are structured in a way that I assume mirrors your process as you developed the code, but that means that for me as a reader, I have to try to follow the "stream of consciousness" of the commits.

    So. Please feed this to me in smaller chunks that contain no merges and that cleanly introduce and explain one idea or feature each, but to save time and effort, please start with the foundational code and don't go any further until I have been able to digest it.

    1. Mario Blažević author

      I'll be happy to answer all your questions.

      This is rather a difficult pull request to review. There's a combination of copy-and-paste duplication of existing code (yuck!) and new concepts that are not explained. In fact, let me back up: I don't even understand the motivation for this module in the first place, so that needs explaining.

      I thought the motivation and much more would be clear from the paper (https://github.com/blamario/monoid-subclasses/wiki/Files/HaskellSymposium2013.pdf) whose presentation you've seen at the Haskell Symposium 2013, I believe. On the other hand, I now realize that the paper is only linked from the monoid-subclasses project, not from the monoid-attoparsec fork, so the connection was not that trivial. Sorry.

      So. Please feed this to me in smaller chunks that contain no merges and that cleanly introduce and explain one idea or feature each, but to save time and effort, please start with the foundational code and don't go any further until I have been able to digest it.

      I'm not sure how smaller chunks would help. The large majority of changes is in the three new modules, the remainder consists of a total of 15 lines of code. I wouldn't recommend trying to understand the new modules based on their change history: just looking at the final state would likely be easier.

      I'd recommend the following approach:

      1. Read the paper, especially sections 3 and 4.
      2. Understand the monoid-subclasses package.
      3. Compare the Data.Attoparsec.Monoid.Internal to Data.Attoparsec.Internal module.
      4. Check the tests and benchmarks, if you wish.
      5. Decide on the course of action.

      As I said, I'm quite open to a rejection; in that case I'll just publish a new package on Hackage. Also, if you find you dislike the merge request as-is but would accept it after some modifications, we can discuss that. Everything's good as long as the fork is not stuck in this limbo any more.

  2. Bryan O'Sullivan repo owner

    Hi, Mario -

    I've taken the time to read your paper. I believe that it is not appropriate to include it in attoparsec right now, and that you should go ahead with publishing it in a separate package.

    If you need components from attoparsec exposed to help you to write the separate package without needing to duplicate code, please let me know (perhaps via pull requests to expose the necessary functionality).

    Thanks.

  3. Mario Blažević author

    If you need components from attoparsec exposed to help you to write the separate package without needing to duplicate code, please let me know (perhaps via pull requests to expose the necessary functionality).

    The only hidden module I can use would be Data.Attoparsec.Internal.Types. Would you object to exposing it, and also to moving the Monoid instance of Parser (which I really don't want) to Data.Attoparsec.Types? I can prepare a pull request if that's OK with you.

  4. Bryan O'Sullivan repo owner

    That's fine, but please name the module Data.Attoparsec.Types.Internal, and mention in the doc comment for the module that instances are not defined in the module, but in its public counterpart.