Updates to dependencies, and small touchups in wijkanders #128

The1Penguin · 2024-06-04T16:43:33Z

No description provided.

Multipacker

Looks good to me

Multipacker · 2024-06-04T17:15:49Z

docker-compose.yml

@@ -1,5 +1,3 @@
-version: '3.7'


Good to see this removed as it is now obsolete.

Multipacker · 2024-06-04T17:21:26Z

src/Config.hs

@@ -31,4 +31,4 @@ defaultConfig = Config False 14 (1000000 * 60 * 30) 5007
 -- TODO: Feel free to bikeshed the function name.


Not enough bikesheding 😉

Much better!

Rembane · 2024-06-04T17:39:43Z

src/Config.hs

@@ -31,4 +31,4 @@ defaultConfig = Config False 14 (1000000 * 60 * 30) 5007
 -- TODO: Feel free to bikeshed the function name.
 reifyConfig
  :: ([Config -> Config], [String], [String]) -> (Config, [String], [String])
-reifyConfig = (& _1 %~ foldl' (flip id) defaultConfig)
+reifyConfig = _1 %~ foldl' (flip id) defaultConfig


Out of curiosity: Is it so that the (& ...) construct doesn't really do anything?

Apparently not, not fully sure why, but HLS complained so I took the Haskell way and got really lazy ;P

Damn legit! Laziness <3

Here, have some types!

λ: :t (&) (&) :: a -> (a -> b) -> b

Then it makes total sense, since we have the function we want to apply, we just apply it instead

Rembane

Good stuff! If it typechecks, ship it!

Rembane · 2024-06-04T17:40:03Z

src/Model/Wijkanders.hs

@@ -90,7 +91,7 @@ getWijkanders d =
    >>> dropWhile (~/= "<strong>")
    >>> removeWhitespaceTags
    >>> partitions (~== "<strong>")
-    >>> mapMaybe ((maybeTagText =<<) . (`atMay` 1))
+    >>> mapMaybe (maybeTagText <=< (`atMay` 1))


malmz

Prima symbolsoppa, LGTM

Multipacker · 2024-06-04T18:26:25Z

src/Config.hs

@@ -31,4 +31,4 @@ defaultConfig = Config False 14 (1000000 * 60 * 30) 5007
 -- TODO: Feel free to bikeshed the function name.


Much better!

The1Penguin · 2024-06-04T19:53:24Z

I cannot believe that this wasn't in the thyme library

The1Penguin · 2024-06-04T19:58:56Z

Oops, forgot to check, now it works. This would fix #33

Multipacker · 2024-06-04T20:33:29Z

src/Util.hs

+  | m < 12    = YearMonthDay  y      (m + 1) (d - len)
+  | otherwise = YearMonthDay (y + 1)  1      (d - len)


Couldn't these just set the day to 0?

Suggested change

| m < 12 = YearMonthDay y (m + 1) (d - len)

| otherwise = YearMonthDay (y + 1) 1 (d - len)

| m < 12 = YearMonthDay y (m + 1) 0

| otherwise = YearMonthDay (y + 1) 1 0

Right, these should be 1 but since they are bounded they will bounce up to being a 1

Ohh I was wondering why they were using different bases for months and days, that explains it

SimonAlling · 2024-06-05T09:27:03Z

Seems like this PR changes multiple completely unrelated things, and that some of the changes might have quite non-obvious implications. Should they really be thought of as a single "conceptual" change? 👀 😊

The1Penguin · 2024-06-05T09:36:13Z

I disagree that a pull request should be seen as a single "conceptual" change. It is one way to approach pull requests, but that is more fit when multiple people are working on the same project at the same time. In this case, it is a project that occasionally gets an update and in this case, I don't see it as problematic, especially since the common way to merge is via rebase.

What is your idea about non-obvious implications? The only one I can think of is the overflow of days, which fixes a bug that has been known about for 6 years.

The1Penguin · 2024-06-05T10:06:41Z

I went ahead and changed so we don't have the time overflow aspect for this pull request, since it matches the history of the pull request in this repo.

SimonAlling · 2024-06-05T11:49:54Z

I disagree that a pull request should be seen as a single "conceptual" change. It is one way to approach pull requests, but that is more fit when multiple people are working on the same project at the same time. In this case, it is a project that occasionally gets an update and in this case, I don't see it as problematic, especially since the common way to merge is via rebase.

Merging by rebasing certainly alleviates the problem. It's obviously more difficult to review many changes at once than one at a time, but the history benefits should at least be preserved then. Good to hear that you've thought it through!

What is your idea about non-obvious implications? The only one I can think of is the overflow of days, which fixes a bug that has been known about for 6 years.

I was thinking of the language version, compiler, libraries etc.

The1Penguin · 2024-06-05T12:43:57Z

I was thinking of the language version, compiler, libraries etc.

Changing the version is something that I believe is a positive change that will make it easier for others to get into the repo. Last time we did such an update was in #120 where we also removed stack for it to be easier to manage.

Having up to date dependencies will also make it easier to manage in the event that there is a breaking change in an update. Not doing incremental updates is one of the reasons that DFoto is in the state that it is (DFoto currently cannot be built).

SKyletoft · 2024-06-05T17:23:46Z

src/Model/Wijkanders.hs

-              )
-            )
+           (or . ([
+                      BL.isPrefixOf (BL8.pack "Med reservation")


We love indentation

SimonAlling · 2024-07-30T09:23:34Z

Changing the version is something that I believe is a positive change that will make it easier for others to get into the repo. Last time we did such an update was in #120 where we also removed stack for it to be easier to manage.

Having up to date dependencies will also make it easier to manage in the event that there is a breaking change in an update. Not doing incremental updates is one of the reasons that DFoto is in the state that it is (DFoto currently cannot be built).

For the record, I’m all for updating dependencies etc. My suggestion was only about doing it in a separate PR.

The1Penguin requested review from Multipacker, SwedishSubmarine and Discomannen June 4, 2024 16:44

Multipacker approved these changes Jun 4, 2024

View reviewed changes

Rembane reviewed Jun 4, 2024

View reviewed changes

Rembane approved these changes Jun 4, 2024

View reviewed changes

The1Penguin requested a review from Multipacker June 4, 2024 18:16

malmz approved these changes Jun 4, 2024

View reviewed changes

The1Penguin force-pushed the touchups branch from d95ce3b to 58ea208 Compare June 4, 2024 18:29

Multipacker approved these changes Jun 4, 2024

View reviewed changes

The1Penguin force-pushed the touchups branch from 5e104ef to ffb80d9 Compare June 4, 2024 18:45

The1Penguin changed the title ~~Updates to dependencies and small touchups in wijkandes~~ Updates to dependencies and small touchups in wijkanders Jun 4, 2024

The1Penguin force-pushed the touchups branch from 2e1286e to 32466e2 Compare June 4, 2024 19:58

The1Penguin changed the title ~~Updates to dependencies and small touchups in wijkanders~~ Updates to dependencies, fix of overflow of days, and small touchups in wijkanders Jun 4, 2024

The1Penguin requested review from Multipacker and Rembane June 4, 2024 20:02

Multipacker reviewed Jun 4, 2024

View reviewed changes

Multipacker approved these changes Jun 4, 2024

View reviewed changes

The1Penguin force-pushed the touchups branch from 757279e to afbd242 Compare June 5, 2024 10:05

The1Penguin changed the title ~~Updates to dependencies, fix of overflow of days, and small touchups in wijkanders~~ Updates to dependencies, and small touchups in wijkanders Jun 5, 2024

The1Penguin self-assigned this Jun 5, 2024

The1Penguin added 2 commits June 5, 2024 19:09

Updates

0dc2469

Workflow change

d70399d

The1Penguin added 4 commits June 5, 2024 19:09

This is cute

8e0986a

Tehee

e30a20c

Formatting

69fca88

Added another cachix cache to readme

6a8e249

The1Penguin force-pushed the touchups branch from afbd242 to 6a8e249 Compare June 5, 2024 17:09

SKyletoft reviewed Jun 5, 2024

View reviewed changes

Indentation goes crazy

f30658c

The1Penguin merged commit 899e274 into main Jun 5, 2024
1 check passed

The1Penguin deleted the touchups branch June 5, 2024 17:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updates to dependencies, and small touchups in wijkanders #128

Updates to dependencies, and small touchups in wijkanders #128

The1Penguin commented Jun 4, 2024

Multipacker left a comment

Multipacker Jun 4, 2024

Multipacker Jun 4, 2024

The1Penguin Jun 4, 2024

Multipacker Jun 4, 2024

Rembane Jun 4, 2024

The1Penguin Jun 4, 2024

Rembane Jun 4, 2024

The1Penguin Jun 4, 2024

Rembane left a comment

Rembane Jun 4, 2024

malmz left a comment

Multipacker Jun 4, 2024

The1Penguin commented Jun 4, 2024

The1Penguin commented Jun 4, 2024

Multipacker Jun 4, 2024

The1Penguin Jun 4, 2024 •

edited

Loading

Multipacker Jun 4, 2024

SimonAlling commented Jun 5, 2024

The1Penguin commented Jun 5, 2024

The1Penguin commented Jun 5, 2024

SimonAlling commented Jun 5, 2024

The1Penguin commented Jun 5, 2024 •

edited

Loading

SKyletoft Jun 5, 2024

The1Penguin Jun 5, 2024

SimonAlling commented Jul 30, 2024

		@@ -31,4 +31,4 @@ defaultConfig = Config False 14 (1000000 * 60 * 30) 5007
		-- TODO: Feel free to bikeshed the function name.

		\| m < 12 = YearMonthDay y (m + 1) (d - len)
		\| otherwise = YearMonthDay (y + 1) 1 (d - len)

Updates to dependencies, and small touchups in wijkanders #128

Updates to dependencies, and small touchups in wijkanders #128

Conversation

The1Penguin commented Jun 4, 2024

Multipacker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rembane left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

malmz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

The1Penguin commented Jun 4, 2024

The1Penguin commented Jun 4, 2024

Choose a reason for hiding this comment

The1Penguin Jun 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimonAlling commented Jun 5, 2024

The1Penguin commented Jun 5, 2024

The1Penguin commented Jun 5, 2024

SimonAlling commented Jun 5, 2024

The1Penguin commented Jun 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimonAlling commented Jul 30, 2024

The1Penguin Jun 4, 2024 •

edited

Loading

The1Penguin commented Jun 5, 2024 •

edited

Loading