search

ThePrimeagen / ts-rust-zig-deez

610 stars 163 forks source link

Refactor haskell lens implementation using State operators #183

Closed ambroslins closed 1 year ago

ambroslins commented 1 year ago

There are specific lens operators for the state monad. I think this implementation is a bit clearer and better shows the power of lenses. However I would like to know what @alexjercan, @newmaidumosa and @Ofenhed think about this before I update the README.

vhladko commented 1 year ago

@alexjercan could you take a look to this ? :)

alexjercan commented 1 year ago

I added some comments with some fixes. Also the rest of the code looks good. Pretty interesting operators :)

alexjercan commented 1 year ago

can you also do make fmt just to use the same formatter everywhere? (there is no fourmolu config in the project, I guess I should probably add one just in case it works with xdg config or something, but I used defaults I think)

Ofenhed commented 1 year ago

Not having used lenses much (as I think they produce confusing code) I can only agree with @alexjercan. The diff was too big (just looking on my phone it looked as if a lot was pure styling), but the operators were very interesting 😊

ambroslins commented 1 year ago

Sorry i forgot to run make fmt on the last commit. Should be fixed now.

I added some comments with some fixes. Also the rest of the code looks good. Pretty interesting operators :)

Where did you add these comments? I can't find them.

I should be able to update the README within the next 24h.

Ofenhed commented 1 year ago

@alexjercan Did you remember to commit the comments? They are all stuck as pending until you send them as either plain comments, accept review, or reject review.

alexjercan commented 1 year ago

Where did you add these comments? I can't find them.

Oh you are right, they were pending. No worries, it was related to the imports, it is all good now.

vhladko commented 1 year ago

Is it good for merge now ?

Ofenhed commented 1 year ago

The README references the old operators, so I would say it should be updated before it's merged. A simple explanation of how .= relates to the already explained a & b .~ c would probably be enough.

alexjercan commented 1 year ago

Is it good for merge now ?

will ping you when it is ready

ambroslins commented 1 year ago

@alexjercan, @Ofenhed. I have updated the README. Feel free to leave suggestions.

And thank you for the feedback on the implementation.

alexjercan commented 1 year ago

Yeah, it looks good to me. @vhladko I think it is ok to merge it