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.

Sign up for GitHub

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

Jump to bottom

Linsen chose to change their api D: #134

Merged
merged 4 commits into from
Sep 24, 2024
Merged

Linsen chose to change their api D: #134

merged 4 commits into from
Sep 24, 2024

Conversation

The1Penguin
Copy link
Contributor

Please fix

Multipacker
Multipacker approved these changes Sep 24, 2024
View reviewed changes
Copy link
Contributor

@Multipacker Multipacker left a comment

Choose a reason for hiding this comment

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

LGTM

@SwedishSubmarine
Copy link
Contributor

Add comment about what magic numbers mean

Rembane
Rembane requested changes Sep 24, 2024
View reviewed changes
Copy link
Contributor

@Rembane Rembane left a comment

Choose a reason for hiding this comment

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

Good stuff! See requests in comments! Keep up the good work!

src/Model/Linsen.hs Outdated
import Data.Thyme ( parseTime
, defaultTimeLocale )
import Data.Thyme ( parseTime )
import Data.Time.Format (TimeLocale (..) )
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you use https://hackage.haskell.org/package/thyme-0.4/docs/Data-Thyme-Format.html#t:TimeLocale instead? So you don't have to add another dependency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, about that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You see, Thyme uses Time as the dependency, which this constructor, but yes, it does reexport it so I will change that

Copy link
Contributor

Choose a reason for hiding this comment

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

Sweet!

test/Main.hs
@@ -84,16 +84,16 @@ main = hspec $ do
s1 <- BL.readFile "test/linsen1.json"
testFun
(Right [ Menu
Copy link
Contributor

Choose a reason for hiding this comment

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

I love the updating of the example.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe I should eat lunch.

src/Model/Linsen.hs
@@ -92,9 +128,9 @@ parse day =

menuParser :: [Value] -> Parser [Menu]
menuParser = pure . (zip [0 :: Integer ..] >=> \case
Copy link
Contributor

Choose a reason for hiding this comment

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

Magic indices <3

These are hard to make less magic. :(

Copy link
Contributor

Choose a reason for hiding this comment

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

I was wrong! :D

Rembane
Rembane approved these changes Sep 24, 2024
View reviewed changes
Copy link
Contributor

@Rembane Rembane left a comment

Choose a reason for hiding this comment

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

This looks good!

src/Model/Linsen.hs
@@ -92,9 +128,9 @@ parse day =

menuParser :: [Value] -> Parser [Menu]
menuParser = pure . (zip [0 :: Integer ..] >=> \case
Copy link
Contributor

Choose a reason for hiding this comment

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

I was wrong! :D

mat-chalmers.cabal Outdated
@@ -55,7 +55,6 @@ library
, word8 == 0.1.3
, extra >= 1.7.16 && <= 1.8
, vector-space >= 0.16 && <0.18
, time >= 1.11 && <1.20
Copy link
Contributor

Choose a reason for hiding this comment

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

\o/

SwedishSubmarine
SwedishSubmarine approved these changes Sep 24, 2024
View reviewed changes
src/Model/Linsen.hs
pattern MeatDish, FishDish, VegDish :: Integer
pattern MeatDish = 3
pattern FishDish = 8
pattern VegDish = 13
Copy link
Contributor

Choose a reason for hiding this comment

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

Vad är oddsen att de kommer ändra dessa igen...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

T-T

@The1Penguin The1Penguin merged commit 7db84d5 into main Sep 24, 2024
1 check passed
@The1Penguin The1Penguin deleted the linsen-my-beloved branch September 24, 2024 12:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Rembane Rembane Rembane approved these changes

@Multipacker Multipacker Multipacker approved these changes

@SwedishSubmarine SwedishSubmarine SwedishSubmarine approved these changes

@rachelambda rachelambda Awaiting requested review from rachelambda

@Discomannen Discomannen Awaiting requested review from Discomannen

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@The1Penguin @SwedishSubmarine @Rembane @Multipacker