Repository navigation
Add the ci flag and fix every warning it turns up - #6
Merged
Merged
Conversation
There was a problem hiding this comment.
Pull request overview
Adds opt-in strict CI compiler warnings across all packages and resolves the resulting warnings.
Changes:
- Adds and enables the
cibuild flag for Stack builds. - Cleans up warnings, dependencies, and deprecated Conduit types.
- Exports additional FTP/TLS commands and bumps package versions.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
stack-base.yaml |
Enables CI flags for all packages. |
ftp-client/test/test.hs |
Resolves test warnings. |
ftp-client/src/Network/FTP/Client.hs |
Exports commands and fixes warnings. |
ftp-client/package.yaml |
Adds CI configuration and bumps version. |
ftp-client/ftp-client.cabal |
Synchronizes generated package metadata. |
ftp-client/CHANGELOG.md |
Documents the release. |
ftp-client-conduit/test/Spec.hs |
Resolves test warnings. |
ftp-client-conduit/src/Network/FTP/Client/Conduit.hs |
Replaces deprecated Conduit aliases. |
ftp-client-conduit/package.yaml |
Adds CI configuration and dependency bounds. |
ftp-client-conduit/ftp-client-conduit.cabal |
Synchronizes generated package metadata. |
ftp-client-conduit/CHANGELOG.md |
Documents the release. |
example/test/Spec.hs |
Adds an explicit module export. |
example/package.yaml |
Adds CI configuration. |
example/example.cabal |
Synchronizes generated package metadata. |
example/app/Main.hs |
Resolves example warnings. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Follows bounded-text, beeline, shrubbery, orb, rollbar-haskell and haskell-non-empty-text, which all carry a manual ci flag holding the strict ghc-options. Two deviations from that list: no henforcer plugin, since this repo has no henforcer.toml and its open imports would fail immediately, and no -Wmissing-import-lists, which would rewrite thirteen imports in the file PR #2 also edits. The flag is manual and defaults off, so nothing reaches Hackage consumers. It is enabled for all three packages in stack-base.yaml, so every rung of the matrix builds with -Werror. Libraries get no ghc-options at all when the flag is off, as before -- an -O2 in the else branch would have imposed it on downstream users, and cabal check says so. Roughly fifty warnings across the four source files. Most were mechanical: unused imports and do-binds, shadowed names, missing local signatures. Three were not: The record update at connectTLS was incomplete because TLSSettings is a sum type and settingDisableCertificateValidation only exists on TLSSettingsSimple. Rebuilding just that constructor keeps it working on crypton-connection 0.3, which lacks the settingClientSupported field a positional call would need. Producer and Consumer are deprecated conduit synonyms, five of them in exported signatures. They expand to `forall i. ConduitT i o m ()` and `forall o. ConduitT i o m r`, not to ConduitT () o m () and ConduitT i Void m r -- those are Source and Sink. Rewriting them with the type variables left free keeps the exported types identical; pinning them to ()/Void would have narrowed the API and broken the internal fusion in mlsd and stor. MonadResource is in five exported signatures but is re-exported by Conduit, so the direct resourcet import counted as unused and -Wunused-packages then rejected the dependency. Taking the name from resourcet explicitly and hiding it from the Conduit import keeps the dependency and its version bound honest. transformers comes out of ftp-client and containers out of example; neither was imported anywhere. Deriving Typeable is dropped -- a no-op since GHC 7.10 that only GHC 9.12 warns about -- and acct, pbsz, prot, ccc and auth are now exported rather than sitting unused, matching every other command wrapper.
cabal check flagged bytestring, conduit and exceptions as having no upper bound. The bounds match what the matrix actually exercises -- conduit 1.3.5 through 1.3.6.1, bytestring 0.11.4.0 through 0.12.2.0, exceptions 0.10.4 through 0.10.12 -- rounded to the next major in each case, and the bytestring and exceptions ranges are spelled the same way ftp-client already spells them. Existing lower bounds are left alone; only the missing upper halves are added. cabal check is now clean on both published packages.
ftp-client gains five exports -- acct, pbsz, prot, ccc and auth -- which is an additive API change, so PVP asks for a minor bump. It also loses the transformers dependency and the no-op Typeable deriving. ftp-client-conduit only gains dependency bounds; the Producer and Consumer rewrite is type-identical because the type variables are left free, so a patch bump covers it.
Replacing the deprecated Producer and Consumer synonyms put ConduitT into the exported signatures, and ConduitT arrived in conduit 1.3. The bound still said >= 1.1, so cabal was free to pick a conduit that cannot compile the module. The bounds audit in the preceding commit deliberately touched only the missing upper halves and left existing lower bounds alone. This is the one lower bound that the same commit's own change invalidated, so it belongs here rather than with the audit. hpack renders >= 1.3 && < 1.4 as ==1.3.*, which is why the generated cabal file reads that way; it is the same constraint and the same spelling already used for ftp-client ==0.5.*. Reported by Copilot on #6. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
onslaughtq
force-pushed
the
add-ci-flag
branch
from
August 25, 2026 00:59
f106388 to
b282da1
Compare
onslaughtq
marked this pull request as ready for review
September 1, 2026 20:37
ysangkok
reviewed
Sep 1, 2026
| - -Wno-unsafe | ||
| - -Wunused-packages | ||
| when: | ||
| - condition: impl (ghc <= 9.3) |
There was a problem hiding this comment.
Why test an unreleased GHC version? Either it should be 9.2 or 9.4.
Author
There was a problem hiding this comment.
@ysangkok I've changed this to be < 9.4 everywhere. Notably, specifying 9.2 wouldn't have worked, since that would excluded 9.2.x versions
impl (ghc <= 9.3) and impl (ghc < 9.4) select the same compilers, since 9.3 was a development version and was never released. Spell the boundary with a version that exists so it does not read as a version being tested. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ysangkok
approved these changes
Sep 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds the
ciflag the other repos use, carrying-Wall -Werrorand the strict warning set, switched on for all three packages instack-base.yaml. It's manual and defaults off, so Hackage consumers never see it.-Wallreported 27 warnings inftp-clientand 22 across the other two. Most were mechanical. Three weren't:The record update in
connectTLSwas incomplete, becauseTLSSettingsis a sum type andsettingDisableCertificateValidationonly exists onTLSSettingsSimple. Rebuilding just that constructor keeps it working on crypton-connection 0.3, which lacks the field a positional call would need.MonadResourceis in five exported signatures of the conduit package butConduitre-exports it, so the direct resourcet import counted as unused, and-Wunused-packagesthen rejected the dependency itself. Taking the name from resourcet and hiding it from theConduitimport satisfies both without dropping a dependency whose types are in the public API.ProducerandConsumerare deprecated conduit synonyms, five of them in exported signatures. They expand toforall i. ConduitT i o m ()andforall o. ConduitT i o m r, not toConduitT () o m ()andConduitT i Void m r— those areSourceandSink. Rewriting them with the type variables left free keeps the exported types identical.Two smaller notes:
-Wmissed-specialisationsnever fired, so no pragmas were needed, and GHC 9.12 is the only version that rejectsderiving Typeable, a no-op since 7.10.This also exports
acct,pbsz,prot,cccandauth, which were defined but never exported.cccandauthcome back out at the top of the stack — neither can work outside the sequencecreateTLSConnectionuses.Stacked:
add-ci-multi-ghc→fix-response-line-partiality→pr-2-multiline-and-handle-exports→add-ci-flag→add-henforcer-fourmolu→fix-audit-findings. Merge bottom-up.