[#45] Rewrite to Multiple Public Libraries #46

chshersh · 2019-10-02T17:25:44Z

Resolves #45

This doesn't compile. I see the following parsing error with this executable stanza:

Errors encountered when parsing cabal file ./containers-backpack.cabal:

containers-backpack.cabal:142:43: error:
unexpected ':'
expecting "-", white space, "(", "hiding", "requires", comma or end of input

  140 |   build-depends:       containers-backpack:{ int-strict, ordered-strict, unordered-strict, contrib }
  141 | 
  142 |   mixins:              containers-backpack:contrib (Map.Contrib.Group as Map.Contrib.Group.Int)
      |                                           ^

Unfortunately, this means that the interface is not usable at the moment until this bug is fixed in cabal. There were some other issues, like problems with having two signatures for readonly and not only containers, but they are solvable, and this one is show-stopper 😞

However, you can have an example of how better it will become!

chshersh · 2019-10-02T17:33:52Z

I've opened a bug report in cabal:

Parse error when trying to use multiple public libraries within mixins haskell/cabal#6281

vrom911

Nice! Thanks a lot for your work, that's great 👏 I would love to see those changes 👍

I have a couple of questions though:

Why cabal.project is not needed anymore?
It's actually nice that there is a central CHANGELOG but how it suppose to work for several standalone libraries?
Could we at least move the nice refactoring parts and merge them in master now, while waiting for the bugs to be resolved?

vrom911 · 2019-10-03T04:43:05Z

src/readonly/Map.hsig

+
+
+
+


Is this a generated file?

No, it's not generated. There were two signatures before: for readonly containers and for modifiable containers. But this doesn't work for some reason in this new solution. So I have only one file now, but haven't deleted old files yet. Will cleanup later 👍

vrom911 · 2019-10-03T04:44:13Z

src/sig/Map.hsig

+update        :: Key k => (a -> Maybe a) -> k -> Map k a -> Map k a
+delete        :: Key k => k -> Map k v -> Map k v
+alter         :: Key k => (Maybe v -> Maybe v) -> k -> Map k v -> Map k v
+


Could we ask GitHub to highlight hsig files as Haskell? 🙂

I also would love to highlight them in my editor 🙂 But I don't know how to do this...

chshersh · 2019-10-03T05:07:57Z

@vrom911 Thanks for your review! Answering your questions:

It was a multipackage project before. But now there's only a single package. So cabal.project is not needed anymore 🙂
If I understand correctly, you can't specify different versions for different multiple public libraries. So I'm not sure how you can specify different versions of sublibraries. This is a new approach and we have to figure out how to support and maintain such libraries...
Yes, this sounds reasonable 👍 Pushing refactorings via a separate PR will make this PR smaller and easier to re-review in future.

Resolves #45

chshersh · 2019-10-15T15:44:23Z

@vrom911 This PR is ready for review! It finally builds locally, the executable is working, all tests are passing, benchmarks are running. Some highlights and summary of changes:

Now instead of many small packages, we have a single package with multiple public libraries.
To depend on a library, you need to write dependency like this:
```
containers-backpack:ord-strict
```
where containers-backpack is the name of this package (we can rename it any time, any suggestions are welcome) and ord-strict is the name of a public library inside this package.
However, sometimes this doesn't work inside the package itself. So I just specify dependency simply as ord-strict. This is due to Cabal bugs.
I merged readonly signatures and modifiable signatures. They complicated things a lot. I left primitive-containers package for now. But probably we should drop support for it eventually. We can't support it completely anyway (it has 5 different Map data types and we support only one at the moment).

Any feedback is appreciated!

vrom911

That's great to see that you made it work and simplified things a lot! Well done 👍

vrom911 · 2019-10-16T05:38:46Z

containers-backpack.cabal

+                       ViewPatterns
+
+-- signatures for containers
+library sig


So there is no difference between internal libraries and libraries to export?

It's my fault, turns out that all these libraries are indeed internal at the moment. To make them public, I needed to add visibility: public field. And, again, the documentation doesn't really help here...

https://www.haskell.org/cabal/users-guide/developing-packages.html#pkg-field-library-visibility
https://www.haskell.org/cabal/users-guide/developing-packages.html#pkg-section-library-library

😞 we need to write more about such things in public so other people at least would be able to find these tricks easier...

vrom911 · 2019-10-16T05:41:20Z

test/Main.hs

+
+
+-- | ShortText is a Text with length [0, 3] chosen "arbitrarily"
+newtype ShortText = ShortText Text


Should this be named field unShortText?

@vrom911 Yes, I guess it should be made according to our style guide. This code was written a long time ago...

vrom911 · 2019-10-16T05:41:47Z

test/Main.hs

+main = do
+  Int.checkLaws (Proxy @Int) (Proxy @V)
+  Ord.checkLaws (Proxy @K) (Proxy @V)
+  Hash.checkLaws (Proxy @K) (Proxy @V)


Could we fix indentation here?

Yes! Sorry, too many changes, forgot to fix everything...

vrom911 · 2019-10-16T05:42:02Z

test/Main.hs

+Note: do not make minimum larger than 0, empty texts are often edge cases.
+-}
+instance Arbitrary ShortText where
+    arbitrary = ShortText . T.pack <$> (choose (0, 3) >>= flip vectorOf arbitrary)


Could we add the signature if possible?

vrom911

Great! Thanks for the massive work!

vrom911 · 2019-10-16T06:26:43Z

containers-backpack.cabal

+-- signatures for containers
+library sig
+  import:              common-options
+  visibility:          public


chshersh added enhancement New feature or request refactoring labels Oct 2, 2019

chshersh requested a review from vrom911 October 2, 2019 17:25

chshersh self-assigned this Oct 2, 2019

chshersh force-pushed the chshersh/45-Rewrite-to-Multiple-Public-Libraries branch from d202996 to c48a96d Compare October 2, 2019 17:28

chshersh mentioned this pull request Oct 2, 2019

Parse error when trying to use multiple public libraries within mixins haskell/cabal#6281

Closed

vrom911 reviewed Oct 3, 2019

View reviewed changes

chshersh mentioned this pull request Oct 3, 2019

Perform some code cleanup and initial refactoring before MPL #47

Merged

chshersh force-pushed the chshersh/45-Rewrite-to-Multiple-Public-Libraries branch 2 times, most recently from 910ff5d to 1108e9e Compare October 15, 2019 15:33

[#45] Rewrite to Multiple Public Libraries

b60babb

Resolves #45

chshersh force-pushed the chshersh/45-Rewrite-to-Multiple-Public-Libraries branch from 1108e9e to b60babb Compare October 15, 2019 15:39

chshersh marked this pull request as ready for review October 15, 2019 15:39

vrom911 reviewed Oct 16, 2019

View reviewed changes

Fix after review

a1ccb7f

chshersh force-pushed the chshersh/45-Rewrite-to-Multiple-Public-Libraries branch from c1617fc to a1ccb7f Compare October 16, 2019 06:16

vrom911 approved these changes Oct 16, 2019

View reviewed changes

containers-backpack.cabal

-- signatures for containers

library sig

import: common-options

visibility: public

Copy link

Member

vrom911 Oct 16, 2019

Choose a reason for hiding this comment

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

👍

vrom911 merged commit 23f052c into master Oct 16, 2019

vrom911 deleted the chshersh/45-Rewrite-to-Multiple-Public-Libraries branch October 16, 2019 06:27

chshersh added the Hacktoberfest https://hacktoberfest.digitalocean.com/ label Nov 5, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#45] Rewrite to Multiple Public Libraries #46

[#45] Rewrite to Multiple Public Libraries #46

chshersh commented Oct 2, 2019

chshersh commented Oct 2, 2019 •

edited

Loading

vrom911 left a comment

vrom911 Oct 3, 2019

chshersh Oct 3, 2019

vrom911 Oct 3, 2019

chshersh Oct 3, 2019

chshersh commented Oct 3, 2019

chshersh commented Oct 15, 2019

vrom911 left a comment

vrom911 Oct 16, 2019

chshersh Oct 16, 2019

vrom911 Oct 16, 2019

vrom911 Oct 16, 2019

chshersh Oct 16, 2019

vrom911 Oct 16, 2019

chshersh Oct 16, 2019

vrom911 Oct 16, 2019

vrom911 left a comment

vrom911 Oct 16, 2019



		-- \| ShortText is a Text with length [0, 3] chosen "arbitrarily"
		newtype ShortText = ShortText Text

[#45] Rewrite to Multiple Public Libraries #46

[#45] Rewrite to Multiple Public Libraries #46

Conversation

chshersh commented Oct 2, 2019

chshersh commented Oct 2, 2019 • edited Loading

vrom911 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

chshersh commented Oct 3, 2019

chshersh commented Oct 15, 2019

vrom911 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

vrom911 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chshersh commented Oct 2, 2019 •

edited

Loading