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

New builder #231

Closed
wants to merge 11 commits into from
Closed

New builder #231

wants to merge 11 commits into from

Conversation

cchalmers
Copy link
Member

Don't merge yet. See diagrams/diagrams-pandoc/pull/1.

@cchalmers cchalmers force-pushed the new-builder branch 2 times, most recently from 559d355 to ab1514a Compare December 19, 2014 18:53
outputSize :: Lens' (Options b v n) (SizeSpec V2 n)

-- | Build a diagram of the given format to the path using the
-- backend's options. The @Maybe String@ returns any errors.
Copy link
Member

Choose a reason for hiding this comment

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

What Maybe String?

@bergey
Copy link
Member

bergey commented Dec 21, 2014

It seems to me that BackendBuild has a lot of overlap with Mainable and DiagramOpts. I understand that this is partly because there's already overlap - DiagramOpts doesn't use SizeSpec, and the relationship to Options b v n isn't very clear.

Can we make this simpler? Maybe we do need two parallel sets of option types, and rendering functions, but I'd like to think otherwise.

@cchalmers
Copy link
Member Author

Part of the problem is some backends (canvas for example) don't have a "save to file" function. And maybe it won't make sense for some backends to have 'SizeSpec V2` option. I agree it's kinda cumbersome to have a class just for this.

@jeffreyrosenbluth
Copy link
Member

This is currently a limitation of blank-canvas. There are plans to rectify it, but it probably won't happen in the immediate future.

mtl >= 2.0 && < 2.3
mtl >= 2.0 && < 2.3,
transformers,
exceptions
Copy link
Member

Choose a reason for hiding this comment

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

We ought to add some version bounds on these.

@cchalmers cchalmers force-pushed the new-builder branch 2 times, most recently from 975f1fc to ace61d0 Compare January 19, 2015 15:51
@cchalmers
Copy link
Member Author

I added a findSandbox function to Diagrams.Util which should be smart enough to find most sandbox (including remote sandboxes, sandboxes in parent folders or sandboxes from DIAGRAMS_SANDBOX variable). Other tools like builder and the linking finding in doc also use it. If anyone gets a chance to try it out let me know if it doesn't work for you.

I also made some changes to the CmdLine loop for a cleaner output.

I'm still unsure the about BackendBuild class.

@bergey
Copy link
Member

bergey commented Mar 10, 2015

@cchalmers Are you hoping to merge this before the 1.3 release, or immediately after?

I'm leaning towards after, but I'm willing to be persuaded. As fiddly as the -doc build system is, I think I'd rather slog through one more release with a tested system, than change it with 3 weeks before we release.

@cchalmers
Copy link
Member Author

I'm not sure. It shouldn't be too much work to get the site working using this. The htmls and pdfs generate the diagrams fine, it just needs integrating with the shake file and the website's style.

That said, I'm not happy with the details of BackendBuild and the whole thing feels unpolished.

Maybe it's best just to fix the current rst tutorials and work on this after.

I could split out the sandbox finder (0756912) and the prettier CmdLine loop (4857193) and merge that before 1.3.

@bergey
Copy link
Member

bergey commented Mar 10, 2015

Splitting out those two changes sounds great.

I'd like to think more about Mainable / DiagramOpts / BackendBuild. Since you're also ambivalent about BackendBuild, doing it after the release seems better.

@bergey
Copy link
Member

bergey commented Nov 13, 2015

I think all of this has been merged except for the BackendBuild class. Per diagrams/diagrams-builder#16 we think we have a better way forward, so I'm closing this. I'll leave the branch around in case I'm wrong about getting diagrams-pandoc to pick builders from PATH.

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