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

Don't change document graphics state when rendering with option dry_run in Text::Formatted::Box #736

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@ingk
Contributor

ingk commented Jun 24, 2014

Rendering a Text::Formatted::Box with the dry_run option led to output in the rendered PDF.

@practicingruby

This comment has been minimized.

Member

practicingruby commented Jun 24, 2014

It looks like this behavior change is desirable, but I'm not comfortable introducing another instance variable here. Maybe pass an optional value into process_options instead?

@ingk

This comment has been minimized.

Contributor

ingk commented Jun 25, 2014

You're right. I changed it to an optional parameter.

@@ -205,11 +205,12 @@ def initialize(formatted_text, options={})
#
def render(flags={})
unprinted_text = []
dry_run = flags[:dry_run] === true

This comment has been minimized.

@practicingruby

practicingruby Jun 25, 2014

Member

We probably don't need this line, as flags[:dry_run] can be used directly in the two other places it is mentioned (213, 224)

@practicingruby

This comment has been minimized.

Member

practicingruby commented Jun 25, 2014

There are many test failures on this pull request... please investigate those and let me know when the build is green again.

@ingk

This comment has been minimized.

Contributor

ingk commented Jun 25, 2014

I just updated the pull request and now the build passes without errors.

@practicingruby

This comment has been minimized.

Member

practicingruby commented Jun 25, 2014

From the looks of it, this will mutate the input data, meaning you can't safely pass the same array to the method twice (i.e. once for a dry run and then again to actually render).

Please work on a solution that does not mutate the inputs.

@ingk

This comment has been minimized.

Contributor

ingk commented Jun 26, 2014

I don't know if I get you right, but the variable text gets a copy of the input data.

def original_text @original_array.collect { |hash| hash.dup } end

normalize_encoding also relies on original_text. So deleting the key color shouldn't be a problem, since in the next iteration without the dry_run option we get a fresh copy of the original input data.

@practicingruby

This comment has been minimized.

Member

practicingruby commented Jun 26, 2014

I see, we do copy the input arguments. This is surprising because original_text is actually a copy, not the original text. 😦

I'm still not sure how good I feel about this solution, though. It's extremely implicit behavior, and so I'm worried about breakage in future changes to this code. Really we should be having explicitly defined behavior for dry run vs. actual rendering, and not adding various checks for if flags[:dry_run] throughout the code.

This isn't your fault, because it is how the code is designed currently. But I think we should give this more thought before merging.

@ingk

This comment has been minimized.

Contributor

ingk commented Jul 8, 2014

I get your point. Is there anything I can do here? Unfortunately I'm not that familiar with the codebase to do such a change.

@practicingruby

This comment has been minimized.

Member

practicingruby commented Jul 16, 2014

Sorry for the delay in response. I'd say if this patch is working for you, just use your fork until we get something similar merged upstream.

@practicingruby

This comment has been minimized.

Member

practicingruby commented Jul 21, 2014

I looked into fixing the underlying design issues for the dry run mechanism, but it's really complicated because the entire formatted text system is pretty tangled up.

So as an alternative I basically applied your same patch in dd92069, but moved the code into a helper method just to make it less likely to be lost or broken during future refactorings.

@practicingruby

This comment has been minimized.

Member

practicingruby commented Jul 21, 2014

@ingk: Because your pull request was accepted, you now have commit access to Prawn. Please see our contributor guidelines, and happy hacking!

@ingk ingk deleted the ingk:bugfix_render_dry_run_graphics_state branch Jun 3, 2015

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