Skip to content

Reset config #1270

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

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Reset config #1270

wants to merge 10 commits into from

Conversation

willbasky
Copy link

@willbasky willbasky commented Jun 6, 2025

Resolve #1058

  • I use reset command instead of unset to prevent ambiguity with existent unset one.

  • I use type checking to keep the strict correspondence between UserSettings' fields and parser of the arguments that user passes.
    Otherwise it is possible to have bad scenarios like

  1. Uncovered fields
data Thing = Thing
    { name :: Maybe String
    , size :: Maybe Int
    , color :: Maybe Int
    }

data Arguments = Name | Size  -- Color constructor is missed
  1. Exceeded arguments
data Thing = Thing
    { name :: Maybe String
    , size :: Maybe Int
    }

data Arguments = Name | Size| Color  -- Color constructor is exceeded

@hasufell
Copy link
Member

hasufell commented Jun 7, 2025

Can you explain what all the type families are for? I'm unable to see why exactly we need it. We have tests for optparse and can just use those to define our expectations.

@willbasky
Copy link
Author

Type Family Purpose Why Necessary
FieldNameOf Maps Key to UserSettings field name Links keys to fields, detects invalid keys at compile-time
HasValidKey Verifies that a key’s field name is in UserSettings Ensures keys are valid, prevents runtime errors for non-existent fields
HasValidKey' Checks if a field name is in a list of field names Supports HasValidKey with type-level list membership test
FieldNames Extracts UserSettings field names via Generics Automates field name derivation, supports validation and coverage checks
FieldNames' Navigates generic representation to fields Handles Generics metadata layers
FieldNames'' Extracts field names from generic field structure Processes nested fields, supports arbitrary record sizes
(++) Concatenates type-level lists Combines field names from nested (:*:) in FieldNames''
CheckCount Ensures equal number of keys and fields Guarantees complete coverage, detects uncovered fields or excess keys
ShowKeys Formats Key list for error messages Improves error readability for CheckCount
ShowFields Formats Symbol list for error messages Improves error readability for CheckCount
AllKeys Lists all valid Key constructors Provides key set for CheckCount to verify coverage

@willbasky
Copy link
Author

willbasky commented Jun 7, 2025

The type checking is used here just to prevent errors that the developer could make by adding new field to UserSettings and missing to update Reset module (for example)

@hasufell
Copy link
Member

hasufell commented Jun 7, 2025

I prefer to not have this complexity in the codebase.

We're not exposing APIs that need rigorous type family code to ensure safety.

We can add tests to:

@hasufell
Copy link
Member

hasufell commented Jun 8, 2025

That looks much better

Vladislav Sabanov added 2 commits June 8, 2025 20:14
Copy link
Collaborator

@dfordivam dfordivam left a comment

Choose a reason for hiding this comment

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

The throwM usage is problematic, other comments are minor nitpicks.

Copy link
Collaborator

@dfordivam dfordivam left a comment

Choose a reason for hiding this comment

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

LGTM

@hasufell
Copy link
Member

hasufell commented Jul 2, 2025

I'll get to this soon.

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.

ghcup config unset
3 participants