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

pick up PR for named routes cookbook again #1726

Draft
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

MangoIV
Copy link
Contributor

@MangoIV MangoIV commented Mar 7, 2024

This PR is supposed to pick up the great work started in #1542 and rebase on current changes as well as apply fixes suggested by the reviewers, potentially also addressing new change requests.

@MangoIV MangoIV marked this pull request as ready for review March 7, 2024 21:45
@tchoutri tchoutri requested review from ysangkok and tchoutri March 7, 2024 21:45
@tchoutri
Copy link
Contributor

tchoutri commented Mar 7, 2024

Thank you very much @MangoIV for this work! Reading the PR content, I feel like we ought to merge the NamedRoutes and Generics cookbooks together, under the NamedRoutes name.

@MangoIV
Copy link
Contributor Author

MangoIV commented Mar 7, 2024

that's a good idea, I will do that then :)

@ysangkok
Copy link
Contributor

@MangoIV Are you still planning to do this?

@MangoIV
Copy link
Contributor Author

MangoIV commented Apr 21, 2024

I am. I think we’re blocked by servant-qc (release, I have updated it) but I don’t know, let me check; will get to it this week perhaps ;)

@tchoutri
Copy link
Contributor

@MangoIV You may rebase your PR on top of master now, I've integrated servant-quickcheck in the monorepo.

@MangoIV
Copy link
Contributor Author

MangoIV commented Apr 22, 2024

Lovely thank you <3

@MangoIV MangoIV force-pushed the inserting_doc_NamedRoutes branch 3 times, most recently from a97a630 to bc22eaf Compare April 26, 2024 10:23
@tfc
Copy link

tfc commented May 5, 2024

Any news/blockers here regarding the merge? I also find this very valuable.

@tchoutri
Copy link
Contributor

tchoutri commented May 5, 2024

@tfc yes, Mango still has to merge the two cookbooks. :)

@MangoIV MangoIV force-pushed the inserting_doc_NamedRoutes branch 2 times, most recently from 2ac385d to 2a52a01 Compare May 20, 2024 16:15
@MangoIV
Copy link
Contributor Author

MangoIV commented May 20, 2024

I have basically dropped the entire generic cookbook in favour of named routes, afaict I have transported all the information that didn't already occur in named routes but I would appreciate a thorough review to confirm.

@MangoIV MangoIV force-pushed the inserting_doc_NamedRoutes branch 2 times, most recently from e90ea2b to 28cdfb6 Compare May 20, 2024 17:04
@MangoIV MangoIV force-pushed the inserting_doc_NamedRoutes branch from 28cdfb6 to f2e3089 Compare May 20, 2024 17:40
@MangoIV MangoIV force-pushed the inserting_doc_NamedRoutes branch from f2e3089 to 812dcf4 Compare May 20, 2024 19:26
, title :: Title
, year :: Year
}
deriving stock Generic
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, how come you didn't have to enable DerivingStrategies? It's not part of GHC2021.

, wai >=3.2
, warp >=3.2

default-language: Haskell2010
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it be simpler to just declare GHC2021 here?

maintainer: haskell-servant-maintainers@googlegroups.com
build-type: Simple
cabal-version: >=1.10
tested-with: GHC ==8.6.5 || ==8.8.3 || ==8.10.1
Copy link
Contributor

Choose a reason for hiding this comment

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

This can't be right, since it's using GHC2021.

@MangoIV
Copy link
Contributor Author

MangoIV commented May 22, 2024

@ysangkok i still have a couple of failures sorry, I should mark it as draft again

@MangoIV MangoIV marked this pull request as draft May 22, 2024 22:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants