support for generating documents with Apache FOP #2

Merged
merged 2 commits into from May 3, 2012

Conversation

Projects
None yet
2 participants
@fmw
Contributor

fmw commented May 3, 2012

I've added basic support for Apache FOP to scratch my own itch (i.e. to be able to generate invoices from Clojure). The current functionality is rudimentary at best, but it does what I need.

@KushalP

This comment has been minimized.

Show comment Hide comment
@KushalP

KushalP May 3, 2012

Owner

I'm not able to merge this commit at the moment and here's why:

  • You've missed out a require from project.clj for FOP
  • Your use of clojure.xml seems to fail locally for me when merging this patch

More importantly, we discussed previously whether you could add smaller pull requests so that it's easier to track the changes through the project. This is not just for my own benefit as a reviewer, but also for those that wish to dig into how the project has grown over time.

Saying that, this looks like a great addition to the project! I especially like the initial table layout you seem to have done. I'd be happy to merge this pull request if you can do the following:

  • Start off with a simple commit getting FOP working (add it to project.clj, write a single test for a base feature, implement it)
  • Then feel free to add the rest of your changes in a single commit if you wish; otherwise commit them adding a test + implementation per commit
  • Make sure to run lein clean and then lein deps followed by lein test to make sure that you've built everything using Leiningen rather than manually including libs
Owner

KushalP commented May 3, 2012

I'm not able to merge this commit at the moment and here's why:

  • You've missed out a require from project.clj for FOP
  • Your use of clojure.xml seems to fail locally for me when merging this patch

More importantly, we discussed previously whether you could add smaller pull requests so that it's easier to track the changes through the project. This is not just for my own benefit as a reviewer, but also for those that wish to dig into how the project has grown over time.

Saying that, this looks like a great addition to the project! I especially like the initial table layout you seem to have done. I'd be happy to merge this pull request if you can do the following:

  • Start off with a simple commit getting FOP working (add it to project.clj, write a single test for a base feature, implement it)
  • Then feel free to add the rest of your changes in a single commit if you wish; otherwise commit them adding a test + implementation per commit
  • Make sure to run lein clean and then lein deps followed by lein test to make sure that you've built everything using Leiningen rather than manually including libs
@fmw

This comment has been minimized.

Show comment Hide comment
@fmw

fmw May 3, 2012

Contributor

I did update project.clj, but forgot to add it to the commit. I've just corrected this.

Everything is working as intended here. Could you be more specific about your problem with clojure.xml? I suppose you mean clojure.data.xml, by the way, because I'm not using clojure.xml in this code. EDIT: I just realized what the problem must have been: that library is also included in the project.clj file I forgot to include.

As to the single commit: everything I added in this patch is connected and pointless to check in or test by itself. There is only one function that uses Apache FOP (write-pdf!). The rest is all XML formatting. It would be possible to single write-pdf! out in a separate commit, but are you sure this is worth it? I understand your point, but while correct in theory this method doesn't really fit the code in question.

Contributor

fmw commented May 3, 2012

I did update project.clj, but forgot to add it to the commit. I've just corrected this.

Everything is working as intended here. Could you be more specific about your problem with clojure.xml? I suppose you mean clojure.data.xml, by the way, because I'm not using clojure.xml in this code. EDIT: I just realized what the problem must have been: that library is also included in the project.clj file I forgot to include.

As to the single commit: everything I added in this patch is connected and pointless to check in or test by itself. There is only one function that uses Apache FOP (write-pdf!). The rest is all XML formatting. It would be possible to single write-pdf! out in a separate commit, but are you sure this is worth it? I understand your point, but while correct in theory this method doesn't really fit the code in question.

@KushalP

This comment has been minimized.

Show comment Hide comment
@KushalP

KushalP May 3, 2012

Owner

That seems like a reasonable request.

Before I hit merge, why did you group this set of changes into fo.clj and not fop.clj?

Owner

KushalP commented May 3, 2012

That seems like a reasonable request.

Before I hit merge, why did you group this set of changes into fo.clj and not fop.clj?

@fmw

This comment has been minimized.

Show comment Hide comment
@fmw

fmw May 3, 2012

Contributor

Because the markup language is called FO (see http://en.wikipedia.org/wiki/XSL_Formatting_Objects). I figured a "fo" module would be a more logical fit because it describes the method of creating PDF files (as opposed to e.g. PDFBox) instead of the tool. Apache FOP is abstracted away in this module, so the most relevant part seems to be the markup language.

Contributor

fmw commented May 3, 2012

Because the markup language is called FO (see http://en.wikipedia.org/wiki/XSL_Formatting_Objects). I figured a "fo" module would be a more logical fit because it describes the method of creating PDF files (as opposed to e.g. PDFBox) instead of the tool. Apache FOP is abstracted away in this module, so the most relevant part seems to be the markup language.

KushalP added a commit that referenced this pull request May 3, 2012

Merge pull request #2 from fmw/master
support for generating documents with Apache FOP.

This is the first initial step showing how Apache FOP integration/abstraction could occur.

@KushalP KushalP merged commit e446b26 into KushalP:master May 3, 2012

@KushalP

This comment has been minimized.

Show comment Hide comment
@KushalP

KushalP May 3, 2012

Owner

That makes sense. Merged.

I'm not going to release another jar just yet, I think we need to play around with the API and have some more tests first.

Thanks!

Owner

KushalP commented May 3, 2012

That makes sense. Merged.

I'm not going to release another jar just yet, I think we need to play around with the API and have some more tests first.

Thanks!

@KushalP

This comment has been minimized.

Show comment Hide comment
@KushalP

KushalP May 3, 2012

Owner

Can you please remove your copyright comments from the top of each file.

I'll instead aim to add an AUTHORS list shortly.

Owner

KushalP commented May 3, 2012

Can you please remove your copyright comments from the top of each file.

I'll instead aim to add an AUTHORS list shortly.

@fmw

This comment has been minimized.

Show comment Hide comment
@fmw

fmw Nov 19, 2012

Contributor

I've separated the fo namespace out into a separate .jar, because I wanted the convenience of being able to add it as a Maven dependency (since it is currently not part of the Camelot jar). See https://github.com/fmw/clj-fo

When and if you decide to release a new jar I can remove the clj-fo project and point it to Camelot (assuming you have time to merge in possible new code I'm adding in the future). It is also fine with me to split the two projects.

Contributor

fmw commented Nov 19, 2012

I've separated the fo namespace out into a separate .jar, because I wanted the convenience of being able to add it as a Maven dependency (since it is currently not part of the Camelot jar). See https://github.com/fmw/clj-fo

When and if you decide to release a new jar I can remove the clj-fo project and point it to Camelot (assuming you have time to merge in possible new code I'm adding in the future). It is also fine with me to split the two projects.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment