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

* Skip drawing the table cells if they contain only the header. #717

Merged
merged 1 commit into from Apr 28, 2014

Conversation

Projects
None yet
3 participants
@donv
Contributor

donv commented Apr 28, 2014

@donv

This comment has been minimized.

Contributor

donv commented Apr 28, 2014

I see the travis specs fail, but none of the failures are related to this change.

@practicingruby

This comment has been minimized.

Member

practicingruby commented Apr 28, 2014

@donv: Those look like Rubocop related issues.

@packetmonkey: At this stage I would really like to avoid failing the build for Rubocop-related issues we haven't already cleared out of the codebase. Can you investigate?

@packetmonkey

This comment has been minimized.

Contributor

packetmonkey commented Apr 28, 2014

Interesting it looks like the failures are coming from Rubocop but when I merged that code I disabled all the cops that caused any failures and are slowly turning them on in PR's that enable the cop and fix any problems.

When I clone your branch and run the tests I don't see the failures on my end.

The UnusedBlockArgument was included in the repo 16 days ago and 0.21.0 was released 4 days ago. So I badly included the rubocop dep in the gemspec without locking to a version. Then the new cop was added which failed on the current code, which is how your simple PR caused failures where a few weeks ago it would have been fine.

I'll lock the gemspec to the specific version of rubocop I tested against, then you should be able to rebase your PR which should make the tests pass.

I'll update again in a few minutes when I have that sorted out for you.

@packetmonkey

This comment has been minimized.

Contributor

packetmonkey commented Apr 28, 2014

@donv I fixed this in bf8bc54 on master. Lets see if that clears up your PR.

@donv

This comment has been minimized.

Contributor

donv commented Apr 28, 2014

There! It should be OK now.

@donv

This comment has been minimized.

Contributor

donv commented Apr 28, 2014

OK to merge?

@practicingruby

This comment has been minimized.

Member

practicingruby commented Apr 28, 2014

I think so, you can merge it! Please make sure to at least put a placeholder link back to this pull request in the MasterChangelog

donv added a commit that referenced this pull request Apr 28, 2014

Merge pull request #717 from prawnpdf/issue_707
* Skip drawing the table cells if they contain only the header.

@donv donv merged commit 4d1afa9 into master Apr 28, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@donv donv deleted the issue_707 branch Apr 28, 2014

@donv

This comment has been minimized.

Contributor

donv commented Apr 28, 2014

Done. Are we using milestones? Should I file this against the next release? 1.1?

@donv donv self-assigned this Apr 28, 2014

@donv donv modified the milestone: 1.0 Blockers Apr 28, 2014

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