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

Create a library and add various options #2

Closed
wants to merge 13 commits into from
Closed

Create a library and add various options #2

wants to merge 13 commits into from

Conversation

cdupont
Copy link
Contributor

@cdupont cdupont commented Sep 15, 2015

  • added library in cabal file
  • added attribute "echo=Above|Below" to get a display of the code
  • added tests

@cchalmers
Copy link
Member

This is great, nice to see this getting some attention. Couple of small things:

  • I personally prefer diagram to diagrams to mark code blocks
  • I'd be nice if Above/Below didn't have to be capitalised.

Otherwise this looks good.

I don't know if you noticed there's another PR #1 (which I started in December?!, I need to start finishing off my PRs) that has extra features and was pretty much ready to merge, but depends on a branch of diagrams-builder that never got finished. It's well commented if you wanted to look at it for ideas.

@bergey
Copy link
Member

bergey commented Sep 15, 2015

@cchalmers: I advised @cdupont that last I heard you weren't planning to finish the diagrams-builder PR, and hence, #1 wasn't likely to get merged. I'm still happy to revisit it. Certainly we need some story for multiple Backends.

I'll try to get through this in the next week, but probably not before Friday.

@cdupont
Copy link
Contributor Author

cdupont commented Sep 16, 2015

@cchalmers: I renamed diagram to diagrams because that matched better the name of the tool. There might be other platforms generating diagrams :)
Regarding the capitalization, I'll fix that.


readEcho :: [(String, String)] -> Maybe Echo
readEcho attrs = case lookup "echo" attrs of
Just e -> Just (read e)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should probably be readMaybe e. Or maybe a custom parser to allow other capitalization.

@bergey
Copy link
Member

bergey commented Oct 2, 2015

@cdupont What's your use-case for the library? I'm happy to split the library part from the executable, it just hadn't occurred to me that anyone would want this.

Right imgName -> Plain [Image [] ((if absolutePath then pathSeparator : imgName else imgName),"")]
let codeBlock = CodeBlock (ident, "haskell":delete "diagrams" classes, attrs) code
let block' = case readEcho attrs of
(Just Above) -> Table [] [AlignLeft] [] [] [[[codeBlock]], [[imgBlock]]]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why a table? We don't currently use a table in the documentation, and I generally try to avoid tables for layout in HTML.

bergey added a commit that referenced this pull request Oct 9, 2015
bergey pushed a commit that referenced this pull request Oct 9, 2015
from #2

add a usage note about running the tests with a cabal sandbox
@bergey
Copy link
Member

bergey commented Oct 9, 2015

I cherry-picked most of the changes, while preserving the command-line parsing and the existing recognized classes.

In particular, I included these changes:

  • build with latest versions of all dependencies
  • split library from executable
  • parse attrs for choice of echo location
  • tests

Thank you for this PR.

@bergey bergey closed this Oct 9, 2015
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.

4 participants