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.

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

There are fixed size of known VCS, implement Representable container. #6432

Closed
phadej opened this issue Dec 14, 2019 · 5 comments · Fixed by #6612
Closed

There are fixed size of known VCS, implement Representable container. #6432

phadej opened this issue Dec 14, 2019 · 5 comments · Fixed by #6612

Comments

@phadej
Copy link
Collaborator

phadej commented Dec 14, 2019

I.e. container such lookup :: RepoType -> VCS a -> a would be total.

One would need to split OtherRepoType String out of RepoType, into separate type.

There're handful of errors to be removed by this refactor.

I think this is newcomer friendly refactoring.


One example of this "issue", is e.g. miss-typing type in source-repository-package:

% cabal build      
cabal: (SourceRepo {repoKind = RepoThis, repoType = Just (OtherRepoType
"gitt"), repoLocation = Just "https://github.com/phadej/tree-diff.git",
repoModule = Nothing, repoBranch = Nothing, repoTag = Just "123", repoSubdir =
Nothing},SourceRepoRepoTypeUnsupported (OtherRepoType "gitt"))

Technically, we don't need to accept unknown RepoTypes to begin with in the cabal.project parser. Thus there's small argument from splitting RepoType into Enum part, and Either String like wrapper.

@v0d1ch
Copy link
Contributor

v0d1ch commented Mar 23, 2020

@phadej how would you like this Either String like wrapper to look?
I am asking just to make sure I do exactly the thing you wanted.

@phadej
Copy link
Collaborator Author

phadej commented Mar 23, 2020

Currently we have

data RepoType = Darcs | Git | SVN | CVS
              | Mercurial | GnuArch | Bazaar | Monotone
              | OtherRepoType String

After we could have

data KnownRepoType = Darcs | Git | SVN | CVS
                   | Mercurial | GnuArch | Bazaar | Monotone

data RepoType = KnownRepoType KnownRepoType
              | OtherRepoType String

@v0d1ch
Copy link
Contributor

v0d1ch commented Mar 24, 2020

Hey @phadej I made the changes and blindly fixed the compiler errors in code and tests.
Here is the diff master...v0d1ch:6432-implement-Representable-container

There is one test that keeps failing and I need some help fixing it:

  Distribution.Utils.Structured
    VersionRange:                                                                         OK
    SPDX.License:                                                                         OK
    LocalBuildInfo:                                                                       FAIL
      tests/UnitTests/Distribution/Utils/Structured.hs:26:
      expected: 2b983b5312a676b13edb7b476c2fd11e
       but got: e2909c4dccc1d2dea065d96aa3d0d915

@phadej
Copy link
Collaborator Author

phadej commented Mar 24, 2020

The Structured tests fails for a reason. We indeed change type definitions. The solution is simply copy the new hash into the test (you need to split it in half and add 0x for hex numbers).

@phadej
Copy link
Collaborator Author

phadej commented Mar 24, 2020

The patch looks fine. I'll comment more when you make a PR. Thanks for picking this issue!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants