Action/Server refactors #391

Profpatsch · 2022-10-10T10:53:06Z

A bunch of refactors that fell out of a study session of the module list generation code.

The original goal (at Munihac) was to change the sort order, so that the home module of a target is listed first, but we didn’t get that far.

So this is a bunch of refactors that do not (or should not) change any behavior. Any changes will be submitted in separate PRs.

Profpatsch · 2022-10-10T11:08:04Z

cc @smatting

ndmitchell

Thanks very much for the PR! I suggest I review/land this one first, before looking at the others, so we can get through them in order. Let me know if that doesn't seem reasonable.

A few notes - I think the use of Wall makes the code worse in a few ways, but better in a bunch, so I think just winding back a few of them makes sense. Overall, looks good and some nice improvements. Thanks!

ndmitchell · 2022-10-16T15:29:16Z

src/Action/Server.hs

@@ -1,7 +1,10 @@
 {-# LANGUAGE ViewPatterns, TupleSections, RecordWildCards, ScopedTypeVariables, PatternGuards #-}
 {-# LANGUAGE OverloadedStrings #-}
+{-# LANGUAGE MultiWayIf #-}
+{-# OPTIONS_GHC -Wall #-}


I generally find this a bad thing to put in the source code directly - what is in Wall changes, and not all the things in there are a good idea.

I didn’t want to fix warnings in the whole codebase for now, so I only scoped it to this module; do you want to just remove -Wall or have something else in mind?

I opted for manually disabling the shadowing warnings & pattern warnings again

src/Action/Server.hs

ndmitchell · 2022-10-16T15:33:42Z

src/Action/Server.hs

            where k = key x

+data UrlOpts = IsHaddockUrl | IsLocalUrl | IsOtherUrl


Would be great if this data type had some comments as to what each of the types of url looked like or represented (I appreciate this was missing before, but now we have the type, it should be easy to add)

I don’t actually know what these mean semantically, I just refactored. I’ll leave it up to you to document the cases :)

Profpatsch · 2022-11-02T15:05:45Z

Okay, I addressed all the comments I think.

ndmitchell · 2022-11-06T12:56:56Z

I seem to be getting failures on newer GHC:

src/Action/Server.hs:217:21: error: [-Wincomplete-uni-patterns, -Werror=incomplete-uni-patterns]
Error:     Pattern match(es) are non-exhaustive
    In a lambda abstraction:
        Patterns of type ‘[Target]’ not matched: []
    |
217 |     forM_ results $ \is@(Target{..}:_) -> do
    |                     ^^^^^^^^^^^^^^^^^^^^^^^^...

Happy with removing the wall, or fixing it. Also wouldn't be averse to using NonEmpty in the right places to fix it too.

Profpatsch · 2022-11-09T10:16:41Z

Ah yeah, must have improved the exhaustiveness checking. Will fix

Profpatsch · 2022-11-09T15:17:35Z

I opted for removing -Wall for now since I think it would interfere with the other PR’s commits too much and lead to a big rebase.

Profpatsch · 2022-11-09T16:40:15Z

ok looks like the others are unrelated

on MacOS:

FAILED ON MAC
{"message":"API rate limit exceeded for 199.7.166.17. (But here's the good news: Authenticated requests get a higher rate limit. Check out the documentation for more details.)","documentation_url":"https://docs.github.com/rest/overview/resources-in-the-rest-api#rate-limiting"}

on Ubuntu:

cabal: Failed to build wai-logger-2.4.0. The failure occurred during the
configure step. The exception was:
dieVerbatim: user error (cabal: '/home/runner/.ghcup/bin/ghc' exited with an
error:
/home/runner/.cabal/store/ghc-9.0.2/cabal-doctest-1.0.9-c5067c39fbeb5bff36b6ed7706ee31a87e1421142e9fe6d94eb8209fc65f5188/lib/libHScabal-doctest-1.0.9-c5067c39fbeb5bff36b6ed7706ee31a87e1421142e9fe6d94eb8209fc65f5188.a(Doctest.o)(.text+0x3066):
error: undefined reference to
'base_GHCziShow_zdfShowZLz2cUZRzuzdcshowList1_closure'
…

The `local` and `haddock` booleans are only used for determining the URLs to generate, so let’s make that clear.

The function does not really deduplicate the elements, it takes and groups. :)

Upstream does not think shadowing should be avoided, so let’s undo the changes to shadowing. Same with incomplete pattern warnings, we’ll just let it crash for now. Also drop the `Is*` prefix for the URL constructors.

CI unfortunately uses -Werror and tests against more modern versions of GHC, so any new errors will only appear on CI.

Profpatsch · 2023-03-19T16:26:13Z

Okay, I might have some time to work on these again, I rebased. I think most of the comments have been fixed, PTAL.

Profpatsch · 2023-03-19T16:35:24Z

Now all CI runs fail because of the copyright year :(

Profpatsch mentioned this pull request Oct 10, 2022

Action/Server: rewrite showFroms to be clearer #392

Open

ndmitchell reviewed Oct 16, 2022

View reviewed changes

Profpatsch force-pushed the action-server-refactors branch from 1cf7774 to 10ecb58 Compare November 2, 2022 15:05

Profpatsch force-pushed the action-server-refactors branch from 10ecb58 to 612583a Compare November 2, 2022 15:06

Profpatsch requested a review from ndmitchell November 3, 2022 08:03

Profpatsch added 8 commits March 19, 2023 17:23

Action/Server: Document grab* functions

cf839b7

Action/Server: Pass url options as one argument

ba4f4c3

The `local` and `haddock` booleans are only used for determining the URLs to generate, so let’s make that clear.

Action/Server: enable -Wall & fix warnings

2a8e632

Action/Server/showFroms: document & upname a little

5602082

Action/Server/showResults: annotate functions in where

11739d3

Action/Server/dedupeTake: rename to takeAndGroup & document

8b76637

The function does not really deduplicate the elements, it takes and groups. :)

Action/Server: Disable unwanted warnings

29f8eb1

Upstream does not think shadowing should be avoided, so let’s undo the changes to shadowing. Same with incomplete pattern warnings, we’ll just let it crash for now. Also drop the `Is*` prefix for the URL constructors.

Action/Server: remove -Wall again, to prevent -Werror killing CI

efd152e

CI unfortunately uses -Werror and tests against more modern versions of GHC, so any new errors will only appear on CI.

Profpatsch force-pushed the action-server-refactors branch from eac22f7 to efd152e Compare March 19, 2023 16:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Action/Server refactors #391

Action/Server refactors #391

Profpatsch commented Oct 10, 2022

Profpatsch commented Oct 10, 2022

ndmitchell left a comment

ndmitchell Oct 16, 2022

Profpatsch Oct 20, 2022

Profpatsch Nov 2, 2022 •

edited

Loading

ndmitchell Oct 16, 2022

Profpatsch Nov 2, 2022

Profpatsch commented Nov 2, 2022

ndmitchell commented Nov 6, 2022

Profpatsch commented Nov 9, 2022

Profpatsch commented Nov 9, 2022

Profpatsch commented Nov 9, 2022

Profpatsch commented Mar 19, 2023

Profpatsch commented Mar 19, 2023

		where k = key x

		data UrlOpts = IsHaddockUrl \| IsLocalUrl \| IsOtherUrl

Action/Server refactors #391

Are you sure you want to change the base?

Action/Server refactors #391

Conversation

Profpatsch commented Oct 10, 2022

Profpatsch commented Oct 10, 2022

ndmitchell left a comment

Choose a reason for hiding this comment

ndmitchell Oct 16, 2022

Choose a reason for hiding this comment

Profpatsch Oct 20, 2022

Choose a reason for hiding this comment

Profpatsch Nov 2, 2022 • edited Loading

Choose a reason for hiding this comment

ndmitchell Oct 16, 2022

Choose a reason for hiding this comment

Profpatsch Nov 2, 2022

Choose a reason for hiding this comment

Profpatsch commented Nov 2, 2022

ndmitchell commented Nov 6, 2022

Profpatsch commented Nov 9, 2022

Profpatsch commented Nov 9, 2022

Profpatsch commented Nov 9, 2022

Profpatsch commented Mar 19, 2023

Profpatsch commented Mar 19, 2023

Profpatsch Nov 2, 2022 •

edited

Loading