contrib/gofpid: wrap Importer

jung-kurt / gofpdf

A PDF document generator with high level support for text, drawing and images

http://godoc.org/github.com/jung-kurt/gofpdf

MIT License

4.31k stars 777 forks source link

contrib/gofpid: wrap Importer #288

Closed mrtsbt closed 5 years ago

mrtsbt commented 5 years ago

Proposal to allow multiple independent Importer instances. Notably, this should improve concurrent use of the library.

wraps Importer from gofpdi library in a simple struct
allow users to obtain multiple Importer instances
keep a default Importer for backwards compatibility
add package level comment

This PR is based on an idea by @ayvan - see https://github.com/jung-kurt/gofpdf/pull/269 for comparison - but includes recent additions and does not include a gofpdiPdf member in the wrapper struct, thus making it easier to retain backwards compatibility without too much code duplication.

@ayvan, @phpdave11 For your information. And please feel free to suggest better approaches or different wordings for the comments.

phpdave11 commented 5 years ago

@ayvan @mrtsbt thank you for your work on this!

@mrtsbt everything looks good. Would you be willing to provide some example code that demonstrates the ability to use gofpdi concurrently?

mrtsbt commented 5 years ago

@phpdave11 I checked in an example that demonstrates the use of an Importer and a test for concurrent use of Importers, based on some local code I used to check for data races. Is that what you had in mind?

jung-kurt commented 5 years ago

Nice work, @mrtsbt! Ready to merge after two small nits are addressed:

$ go vet -all
# github.com/jung-kurt/gofpdf/contrib/gofpdi
./gofpdi_test.go:12:1: ExampleGofpdiImporter refers to unknown identifier: GofpdiImporter                                                                                       

$ goreportcard-cli  -v
...
ineffassign: 50%
        gofpdi_test.go
                Line 53: warning: ineffectual assignment to tpl (ineffassign)

mrtsbt commented 5 years ago

@jung-kurt Sorry about that. Hopefully the checks pass now? I didn't know about goreportcard-cli till now - though I'd better take a look at it, obviously :) Is the name of the test okay, though?

jung-kurt commented 5 years ago

Great -- all checks pass except for a license. That's something @phpdave11 can place at some point. I assume it is MIT like gofpdi.

goreportcard-cli is pretty nice for catching a lot of little things. However, it gets a little too excited about the number of case statements in a switch block so a lot of very straightforward constructions are called out as too "cyclomatically" complex.

I assume this merge means that #269 can be closed?

phpdave11 commented 5 years ago

Yes #269 can be closed now that this PR has been merged. Thanks!

ayvan commented 5 years ago

Yes #269 can be closed now that this PR has been merged. Thanks!

closed!