search

wf49670 / ppgen

Post-processing generator for DP
6 stars 4 forks source link

drop cap variable argcount #69

Closed ghost closed 9 years ago

ghost commented 9 years ago

request review by Walt and then merge into develop.

ghost commented 9 years ago

I'm not sure if I am supposed to close that pull request and open another one since I've pushed a new "dropcap" revision onto github. It looks like my "corrected doDropImage" work is in here. It corrects the indentation error you spotted. Walt can you review it and merge into develop?

wf49670 commented 9 years ago

On 11/22/2014 4:56 AM, rfrank wrote:

I'm not sure if I am supposed to close that pull request and open another one since I've pushed a new "dropcap" revision onto github. It looks like my "corrected doDropImage" work is in here. It corrects the indentation error you spotted. Walt can you review it and merge into develop?

It looks good to me, Roger.

I do not yet seem to have the necessary privileges at this point to merge things into your repository, nor to close pull requests. Have you added wf49670 as a collaborator? (Settings in the right-hand sidebar, then the Collaborators tab.)

Thanks, Walt

ghost commented 9 years ago

I do not yet seem to have the necessary privileges at this point…

I’ve added you as a collaborator with full access to the repository.

—Roger

wf49670 commented 9 years ago

On 11/22/2014 4:56 AM, rfrank wrote:

I'm not sure if I am supposed to close that pull request and open another one since I've pushed a new "dropcap" revision onto github. It looks like my "corrected doDropImage" work is in here. It corrects the indentation error you spotted. Walt can you review it and merge into develop?

Merged.

Since then, though, Denis has pointed out that we have an inconsistency.

.di with 4 arguments has filename, width, height, adjustment but .di with 3 arguments has filename, height, adjustment

There was a misleading comment in the code indicating that the order for the 4 argument case was height then width, but the code was (and is) width then height.

Should it be height for the 3 argument case or width? Especially since for .il we specify width?

Regards, Walt

ghost commented 9 years ago

Walt, if you agree, lets make it consistent so there is less for the PPer to remember. Let's make it width. Can you do that or do you want me to?

wf49670 commented 9 years ago

On 11/22/2014 9:10 PM, rfrank wrote:

Walt, if you agree, lets make it consistent so there is less for the PPer to remember. Let's make it width. Can you do that or do you want me to?

I can do it. I just wanted to make sure you agreed that we should go for consistency, as I didn't know if you had a specific reason for deciding to go with height vs width for the new .di case.

Thanks, Walt