search

prawnpdf / pdf-core

Implements low level PDF features for Prawn (experimental)
Other
23 stars 38 forks source link

Reduce memory allocations and enhance performance #45

Closed gettalong closed 2 years ago

gettalong commented 3 years ago

See description in the linked PR for details.

petergoldstein commented 2 years ago

@gettalong Would you be able to rebase this PR? I want to see what the specs look like on a rebased PR

gettalong commented 2 years ago

@petergoldstein Was already done by @pointlessone, from a performance standpoint there should be no difference.

petergoldstein commented 2 years ago

@gettalong Looks like we're getting a couple of Rubocop failures. That's what's causing the test fail. Could you remediate those?

petergoldstein commented 2 years ago

@pointlessone If you do another rebase the outstanding issues should be easier to see.

gettalong commented 2 years ago

@gettalong Looks like we're getting a couple of Rubocop failures. That's what's causing the test fail. Could you remediate those?

So, I tried and it seems that I can't push anymore: "ERROR: Permission to prawnpdf/pdf-core.git denied to gettalong.".

petergoldstein commented 2 years ago

Ok @gettalong . Any chance you'd be willing to open a new PR from a fork with the Rubocop fixes? I think it makes sense to get this in.

gettalong commented 2 years ago

Ok @gettalong . Any chance you'd be willing to open a new PR from a fork with the Rubocop fixes? I think it makes sense to get this in.

Nope, can't create a new branch here anymore.

Anyways, the second offense can easily be fixed by using single apostrophes, the first one - mutated constant - probably needs an exception in the Rubocop config for that file and cop?

pointlessone commented 2 years ago

@gettalong I think that's sorted now.

petergoldstein commented 2 years ago

@gettalong I think an inline exception for the first issue using # rubocop:disable Style/MutableConstant would be fine.

pointlessone commented 2 years ago

Inline exception is OK but maybe it's better to make it an instance variable? I feel like permanent cache might be considered a memory leak.

gettalong commented 2 years ago

@pointlessone So, this should be ready to go.