Skip to content

Fix trace functions not pretty-printing as expected#20

Merged
cdepillabout merged 3 commits intocdepillabout:masterfrom
yigitozkavci:fix-issue-19
Jan 10, 2018
Merged

Fix trace functions not pretty-printing as expected#20
cdepillabout merged 3 commits intocdepillabout:masterfrom
yigitozkavci:fix-issue-19

Conversation

@yigitozkavci
Copy link
Copy Markdown
Contributor

Fixes #19

Summary

This PR adds NoColor counterparts of trace functions, and implement tests on them (because testing colored output is awkward).

Notes

@cdepillabout There is a warning introduced with this PR:

warning: [-Wunused-imports]
    The qualified import of ‘Data.Map’ is redundant
      except perhaps to import instances from ‘Data.Map’
    To import instances alone, use: import Data.Map()

Even though I use it for Doctest, I get this warning. I see we didn't use a non-prelude library for our doctests before, but I want to handle a case with Map since @igrep noticed the bug with that data structure.

We can overcome this warning with {-# OPTIONS_GHC -fno-warn-unused-imports #-} pragma on this module. What do you think?

@cdepillabout
Copy link
Copy Markdown
Owner

@yigitozkavci Thanks for working on this.

There is a warning introduced with this PR:

warning: [-Wunused-imports]
   The qualified import of ‘Data.Map’ is redundant
     except perhaps to import instances from ‘Data.Map’
   To import instances alone, use: import Data.Map()

Even though I use it for Doctest, I get this warning. I see we didn't use a non-prelude library for our doctests before, but I want to handle a case with Map since @igrep noticed the bug with that data structure.

We can overcome this warning with {-# OPTIONS_GHC -fno-warn-unused-imports #-} pragma on this module. What do you think?

You should be able to import modules within the doctests.

Here's an example doctest showing this:

>>> import Data.List (nub)
>>> nub [1,2,3,1,1,1]
>>> [1,2,3]

Although, I don't necessarily think it is that important to use Data.Map. If you just want to use something like a list to write the tests, I think that should be fine as well.

@yigitozkavci
Copy link
Copy Markdown
Contributor Author

That's perfect, first time using doctest, that's why :) Did the necessary changes 👍

@cdepillabout
Copy link
Copy Markdown
Owner

Okay, thanks. I'll merge this when the tests pass.

@yigitozkavci
Copy link
Copy Markdown
Contributor Author

Also, I didn't add the @since annotations since I don't know the next release version. Can you add those? @cdepillabout

@cdepillabout cdepillabout merged commit 6168a2b into cdepillabout:master Jan 10, 2018
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.

2 participants