-
Notifications
You must be signed in to change notification settings - Fork 52
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
Split swarm libraries #1069
Split swarm libraries #1069
Conversation
We also need to make sure to update |
@@ -11,7 +11,7 @@ | |||
-- | |||
-- These failures are often not fatal and serve | |||
-- to create common infrastructure for logging. | |||
module Swarm.TUI.Model.Failure where | |||
module Swarm.Game.Util.Failure where |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kostmo FYI I had to move the module back to the Game just to compile, but the change in #1116 is better (no Util
directory and less code in the Util
module itself) so I will rebase once merged. 👍
It seems that I am also susceptible to making silly Util
modules when desperate. 😅
Splitting the libraries with Cabal has been a great learning experience for me, so I hope not to repeat my mistake from #982 where I put this module in TUI in the first place. 🤦
@@ -295,7 +625,7 @@ test-suite swarm-integration | |||
benchmark benchmark | |||
import: stan-config, common, ghc2021-extensions | |||
main-is: Benchmark.hs | |||
hs-source-dirs: bench | |||
hs-source-dirs: test/bench |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@byorgey I moved the app
and bench
directories to src
and test
respectively.
Sometimes I forgot to recompile the benchmark until the CI told me, so I hope seeing it with other tests will remind me about it. 🙂
I did not save file before commit
- name: checkout | ||
uses: actions/checkout@v2 | ||
uses: actions/checkout@v3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Speaking of CI, it looks like regenerating this file will get rid of GitHub warnings about deprecated actions. 🙂
Let's hope it will all work nicely. 🤞
@byorgey @kostmo this will be ready to merge after #1116, please take a look. 🙂 If there are any old PR you wish to resurrect that can not be rebased just because of this PR, I will happily rebase them myself rather than let this wait for all PRs to be slowly merged. However, because this is almost purely moving modules into library folders, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still mostly think that this is a good idea but I am still a bit concerned about the maintenance burden of managing package versions, doing releases, etc. What's our plan for updating the release action?
StrictData | ||
|
||
|
||
library |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why doesn't this library
have a name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It did not before, it is simply swarm
. I am not 100% clear about whether we need this or if swarm-app
would be better or maybe if it could even merge with swarm-exe
.
I do not think we need to do anything about this right now.
@@ -79,12 +80,113 @@ common ghc2021-extensions | |||
-- Not GHC2021, but until we get \cases we use \case a lot | |||
LambdaCase | |||
|
|||
library | |||
|
|||
library boolexpr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's our plan for boolexpr
? I don't think we can have an internal library called boolexpr
when there is already a package on Hackage with that name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It turns out we can... at least until we try to publish it on Hackage. 😄
Do we want to rename it swarm-boolexpr
and support it indefinitely @kostmo?
I like that it is a separate package so we at least know that it does not have any Swarm dependency and could be upstreamed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@byorgey the release action can stay exactly as it is. All commands it uses stay the same, just as you can still use the same Stack/Cabal commands as before.
|
OHHHH, I just realized what my confusion was. I thought by doing this we were going to have to upload a bunch of separate packages to Hackage, like If that's true, then none of my concerns are valid (e.g. version number management, release tooling, and what we will do with |
First, yes. Second, I am not sure. The Hackage release is still just But I think it will actually publish the libraries separately and also publish |
But |
Yes, probably so. We want someone who depends on the |
We should probably try running Haddock on this locally to make sure that it produces something reasonable. |
@byorgey I tested it with Haddock and it showed all modules. 🙂 Feel free to test it yourself and with Stack. Otherwise, I will have to download Stack to test it myself. 😄 |
I tried it with
...and then it proceeds to build everything perfectly. 🤷 |
Also: * adds a script to view locally-generated Haddocks. * Describes module organization as per #1069 (comment) in the toplevel cabal package description
-Wunused-packages
to keep the dependencies minimalThis is how our dependencies look like and the libraries should enforce that we keep everything well ordered:
On a high level, the libraries look like this: