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

Fixes #1786: Moved marshalling code to own namespace #2193

Merged
merged 6 commits into from
May 26, 2021

Conversation

mmhat
Copy link
Collaborator

@mmhat mmhat commented May 24, 2021

All decoding related code was moved to Dhall.Marshal.Decode and the
encoding related code to Dhall.Marshal.Encode respectively.

Additionally some minor documentation issues were fixed.

Fixes #1786

mmhat added 4 commits May 24, 2021 18:19
All decoding related code was moved to Dhall.Marshal.Decode and the
encoding related code to Dhall.Marshal.Encode respectively.

Additionally some minor documentation issues were fixed.
@mmhat
Copy link
Collaborator Author

mmhat commented May 24, 2021

Hm, Hydra fails with

[1 of 1] Compiling Main             ( benchmark/parser/Main.hs, dist/build/dhall-parser/dhall-parser-tmp/Main.o )
Linking dist/build/dhall-parser/dhall-parser ...
haddockPhase
Error: Incomplete haddocks

but my local Haddock builds look fine. What is going on here?
@Gabriel439 The last time I ran into issues concerning Hydra+Haddock your feedback resolved the issue (I cannot remember what it was exactly). Do you have any idea what went wrong this time?

General question: How do I debug problems like this? The log does not give me any clue what went wrong.

@sjakobi
Copy link
Collaborator

sjakobi commented May 26, 2021

@mmhat In the case of this Hydra failure, grepping for "Incomplete haddocks" would give you a hint:

failOnMissingHaddocks = drv:
if compiler == defaultCompiler
then
drv.overrideAttrs
(old: {
postHaddock = (old.postHaddock or "") + ''
! (./Setup haddock 2>&1 | grep --quiet 'Missing documentation for:\|Warning:.*is out of scope') || (echo "Error: Incomplete haddocks"; exit 1)
'';
}
)
else
drv;

But it looks like you've already figured it out! :)

What's the motivation for creating the new namespace BTW? Do you have future plans for it?

@mmhat
Copy link
Collaborator Author

mmhat commented May 26, 2021

@sjakobi The motivation behind the namespaces were the following: The Dhall module in its current form has almost 3000 lines, most of them related to marshalling. Since such a huge module is kind of hard to navigate (Well, at least I had troubles the last time I contributed.) the idea was to move the decoding/encoding code to an own namespace. The goal is (was?) to end up with a clearer separation of concerns and maybe even improved compile times during development due to smaller compilation units.
As you may have noticed this PR does not (yet) implement the original module structure displayed in #1786 due to the tight coupling of the generic code and the marshalling type classes. I have to look a bit more into this if it is really worth splitting the Dhall.Marshal.{Decode,Encode} modules into smaller submodules, but I'd rather do that in a separate PR.

Regarding the Hydra+Haddock issue: Yeah, I finally remembered what caused the issue the last time I ran into this :) Since I could not find any documentation on the Haddock checks Hydra is performing I will open a PR in order to add it to our CONTRIBUTION docs after I am done with this one.

@mmhat mmhat requested a review from sjakobi May 26, 2021 11:04
@Gabriella439 Gabriella439 merged commit 3a42c98 into dhall-lang:master May 26, 2021
@Gabriella439
Copy link
Collaborator

@mmhat: Thank you for doing this! 🙂

@mmhat mmhat deleted the 1786-marshaling-namespace branch May 26, 2021 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move marshalling code in own module namespace
3 participants