Commit 79cd7d4
committed
Merge branch 'develop-spec-arg'. Close #446.
**Description**
The type `Copilot.Language.Stream.Arg` is defined in the same module as
a `Stream`, but nothing in that module uses an `Arg`. Architecturally,
that type doesn't belong in that module, but, rather, should be listed
in `Copilot.Language.Spec`, where it is used in the definition of
`Trigger`.
Consequently, `Copilot.Language.Stream.Arg` should be moved to
`Copilot.Language.Spec`.
As per our internal policy of waiting 3 versions from deprecation until
a public interface declaration can be hidden, this definition should be
deprecated in `Copilot.Language.Stream.Arg` and re-published from
`Copilot.Language.Spec`. However, Haskell offers no mechanism to do
that. If we deprecate it in `Copilot.Language.Stream.Arg`, we cannot
make that warning disappear when imported from `Copilot.Language.Spec`,
even with `{-# OPTIONS_GHC -fno-warn-deprecations #-}`.
**Type**
- Bug: low cohesion in `Copilot.Language.Stream`.
**Additional context**
None.
**Requester**
- Ivan Perez
**Method to check presence of bug**
This is a bug in architecture and design. There is no way to check for
this bug automatically.
There are signs of this bug. For example, running a search on the
codebase indicates that nothing inside `Stream` uses `Arg`, but that is
not totally uncommon in modules that define datatypes.
In this case, it is important to understand that, conceptually, an `Arg`
only makes sense in the context of a `Trigger`, so they belong together.
Because `Trigger` is defined in `Copilot.Language.Spec`, then so should
`Arg`.
We can, however, check that it is defined in `Spec`, as follows:
```Dockerfile
FROM ubuntu:trusty
RUN apt-get update
RUN apt-get install --yes software-properties-common
RUN add-apt-repository ppa:hvr/ghc
RUN apt-get update
RUN apt-get install --yes ghc-8.6.5 cabal-install-2.4
RUN apt-get install --yes libz-dev
ENV PATH=/opt/ghc/8.6.5/bin:/opt/cabal/2.4/bin:$PWD/.cabal-sandbox/bin:$PATH
RUN cabal update
RUN cabal v1-sandbox init
RUN cabal v1-install alex happy
RUN apt-get install --yes git
SHELL ["/bin/bash", "-c"]
CMD git clone $REPO && cd $NAME && git checkout $COMMIT && cd .. \
&& cabal v1-install copilot/copilot-**/ \
&& ! cabal v1-exec -- runhaskell <<< "import Copilot.Language.Stream (Arg); main :: IO (); main = return ()" \
&& cabal v1-exec -- runhaskell <<< "import Copilot.Language.Spec (Arg(Arg)); main :: IO (); main = return ()" \
&& echo "Success"
```
Command (substitute variables based on new path after merge):
```
$ docker run -e "REPO=https://github.com/Copilot-Language/copilot" -e "NAME=copilot" -e "COMMIT=<HASH>" -it copilot-verify-446
```
**Expected result**
Running the script above prints Success, indicating that `Arg` is
defined in and exported from `Copilot.Language.Spec`, and not
`Copilot.Language.Stream`.
**Solution implemented**
Move `Arg` from `Copilot.Language.Stream` to `Copilot.Language.Spec`.
Adjust imports accordingly.
**Further notes**
We provide the function `arg` precisely so that people do not use the
constructor `Arg` directly.5 files changed
Lines changed: 10 additions & 7 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
1 | 4 | | |
2 | 5 | | |
3 | 6 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
14 | 14 | | |
15 | 15 | | |
16 | 16 | | |
17 | | - | |
| 17 | + | |
18 | 18 | | |
19 | 19 | | |
20 | 20 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
17 | 17 | | |
18 | 18 | | |
19 | 19 | | |
20 | | - | |
| 20 | + | |
21 | 21 | | |
22 | 22 | | |
23 | 23 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
22 | 22 | | |
23 | 23 | | |
24 | 24 | | |
| 25 | + | |
25 | 26 | | |
26 | 27 | | |
27 | 28 | | |
| |||
198 | 199 | | |
199 | 200 | | |
200 | 201 | | |
| 202 | + | |
| 203 | + | |
| 204 | + | |
| 205 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
9 | 9 | | |
10 | 10 | | |
11 | 11 | | |
12 | | - | |
13 | 12 | | |
14 | 13 | | |
15 | 14 | | |
| |||
48 | 47 | | |
49 | 48 | | |
50 | 49 | | |
51 | | - | |
52 | | - | |
53 | | - | |
54 | | - | |
55 | 50 | | |
56 | 51 | | |
57 | 52 | | |
| |||
0 commit comments