Skip to content
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

Adding support for colspan to tables #100

Merged
merged 1 commit into from
Apr 30, 2019

Conversation

weaverryan
Copy link
Collaborator

@weaverryan weaverryan commented Mar 8, 2019

I know the table parsing doesn't support many complex things. But, I've found a few "still-simple" use-cases that aren't parsing correctly yet.

Reference for this stuff: http://docutils.sourceforge.net/docs/ref/rst/restructuredtext.html#grid-tables

Unless I've made a dumb mistake (possible, I've been staring at tables), the actual markup in these tests should be (in spirit) what Sphinx is rendering (minus things we know are different, like classes, colgroup, etc).

@weaverryan weaverryan force-pushed the failing-test-case-table-structure branch from affeaf4 to 03fcd65 Compare March 8, 2019 21:47
@weaverryan weaverryan changed the title Apparently you can use different separators inside a table Failing test cases for more table types Mar 8, 2019
@weaverryan weaverryan force-pushed the failing-test-case-table-structure branch 2 times, most recently from fca5ef2 to 0367500 Compare March 8, 2019 21:51
@weaverryan
Copy link
Collaborator Author

This is still a WIP. I've done a ton of cleanup, and most of the work now will be in TableNode, to get the tests to pass when running:

./vendor/bin/phpunit tests/Functional/FunctionalTest.php --filter=table

@weaverryan weaverryan force-pushed the failing-test-case-table-structure branch 2 times, most recently from 04c2c8b to 291a49d Compare April 27, 2019 02:29
@weaverryan weaverryan changed the title Failing test cases for more table types Fixing tables. Apr 27, 2019
@weaverryan weaverryan force-pushed the failing-test-case-table-structure branch from 291a49d to d6fbd2d Compare April 27, 2019 02:30
lib/HTML/Renderers/TableNodeRenderer.php Show resolved Hide resolved
lib/NodeFactory/DefaultNodeFactory.php Show resolved Hide resolved
use function trim;
use function utf8_encode;

class TableColumn
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This, along with TableRow help TableNode work with meaningful objects, instead of a ton of arrays.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make this class final if you can.

}
}

private function compileSimpleTable() : void
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The old logic for turning the raw config/lines into a meaningful $this->data property were completely removed. I couldn't make any sense of them. I've replaced them with 2 entirely separate functions for the "simple table" and the "pretty/grid table". Each of these is super unfancy, because I want them to be readable and because algorithms are hard for me :).

Second row | Other col | Other col
Third row | Other col | Last col

in file: "(unknown)"
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

File was renamed and made more interesting

@weaverryan weaverryan force-pushed the failing-test-case-table-structure branch 2 times, most recently from 6e859f5 to fbba2f6 Compare April 27, 2019 02:48
@weaverryan
Copy link
Collaborator Author

CI is failing on nightly, which is unrelated.
Scrutinizer failures all look false.

This is ready to go :)

@jwage
Copy link
Member

jwage commented Apr 29, 2019

Can you make all new classes final if they are not intended to be extended?

Copy link
Member

@jwage jwage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few changes and questions/comments. Looks good. Thanks!

lib/NodeFactory/NodeFactory.php Outdated Show resolved Hide resolved
use function trim;
use function utf8_encode;

class TableColumn
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make this class final if you can.

lib/Nodes/Table/TableColumn.php Show resolved Hide resolved
lib/Nodes/TableNode.php Outdated Show resolved Hide resolved
lib/Nodes/TableNode.php Outdated Show resolved Hide resolved
lib/Nodes/TableNode.php Outdated Show resolved Hide resolved
tests/ErrorManagerTest.php Show resolved Hide resolved
@jwage jwage changed the title Fixing tables. Adding support for colspan to tables Apr 29, 2019
This adds several objects, which replace arrays, for more clarity
It also introduces features that weren't possible before, like colspans.
@weaverryan weaverryan force-pushed the failing-test-case-table-structure branch from fbba2f6 to 457e0fb Compare April 30, 2019 14:19
@weaverryan
Copy link
Collaborator Author

Updates made!

@jwage jwage added the Enhancement New feature or request label Apr 30, 2019
@jwage jwage added this to the 1.0.0 milestone Apr 30, 2019
Copy link
Member

@jwage jwage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@jwage jwage merged commit 59e913f into doctrine:master Apr 30, 2019
@weaverryan weaverryan deleted the failing-test-case-table-structure branch April 30, 2019 23:31
@greg0ire greg0ire removed this from the 1.0.0 milestone Mar 7, 2021
@greg0ire greg0ire added this to the 0.3.0 milestone Mar 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants