Improve usability on 'ghcup config add-release-channel' #781
Conversation
Fixes #751 (or so I hope).
| -- appends the element to the end of the list, but also removes it from the existing list | ||
| appendUnique :: Eq a => [a] -> a -> [a] | ||
| appendUnique xs' e = go xs' | ||
| where | ||
| go [] = [e] | ||
| go (x:xs) | ||
| | x == e = go xs -- skip | ||
| | otherwise = x : go xs |
app/ghcup/GHCup/OptParse/Config.hs
Outdated
| r <- runE @'[DuplicateReleaseChannel] $ do | ||
| case urlSource settings of | ||
| AddSource xs -> do | ||
| when (not force && Right uri `elem` xs) $ throwE (DuplicateReleaseChannel uri) |
There was a problem hiding this comment.
So, if I understand correctly, addition sequence a.b.a will now error unless forced.
But also a.a will error, but this is in effect the same as a, because the action of adding a fixed channel at the end is idempotent (if we ignore the extra entry in xs).
I am a bit worried about the error in such don't-care situations. This could lead to regressions for uses of ghcup in scripts.
But I understand that just ignoring duplicate additions with a warning (unless force) is also not a backwards-compatible solution, and even worse than a hard error.
Could one stick with a warning just in case the last channel is added again? In these cases we know that adding it again is redundant.
There was a problem hiding this comment.
Yes, I think we can skip error if the last element equals the new element and simply do nothing.
app/ghcup/GHCup/OptParse/Config.hs
Outdated
| lift $ doConfig (defaultUserSettings { uUrlSource = Just $ AddSource [Right uri] }) | ||
| pure () | ||
| OwnSource xs -> do | ||
| when (not force && Right uri `elem` xs) $ throwE (DuplicateReleaseChannel uri) |
|
conceptually, is the configuration a Map from keys to config values wrapped in the First Monoid https://hackage.haskell.org/package/base-4.17.0.0/docs/Data-Monoid.html#t:First by default, and the --force flag would correspond with the Last monoid? https://hackage.haskell.org/package/base-4.17.0.0/docs/Data-Monoid.html#t:Last roughly |
|
@cartazio no idea. Never used those.
Say:
Both have duplicate keys (GHC versions). If you have configured the order If you now say add A with current GHCup, you get With the new patch, ghcup will error out. With Does that make sense? |
|
I will admit I’m even more confused about the current semantics then :)
I understand that for non conflicting keys, it should be a union.
What’s the current shadowing override rule on conflict and what’s the new
one?
…On Wed, Feb 15, 2023 at 8:55 PM Julian Ospald ***@***.***> wrote:
@cartazio <https://github.com/cartazio> no idea. Never used those.
--force does not have an effect on which keys in the nested Maps are
overwritten, it only changes the overwrite order.
Say:
- A: ghcup-vanilla-0.0.7.yaml
- B: ghcup-0.0.7.yaml
Both have duplicate keys (GHC versions).
If you have configured the order [ A, B ], then B takes precedence.
If you now say *add A* with current GHCup, you get [ A, B, A ] which is
the same as [ B, A ] just more inefficient.
With the new patch, ghcup will error out.
With --force, you'll get [ B, A ] (as opposed to [ A, B, A ]).
Does that make sense?
—
Reply to this email directly, view it on GitHub
<#781 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAABBQRRNFKSHZW3OKPY6DTWXWCKPANCNFSM6AAAAAAUZIPMBA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Duplicate keys are overwritten right biased, no matter whether you use |
Fixes #751 (or so I hope).
@andreasabel
Test:
Result: