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 test case for sections closing too far #74

Merged
merged 1 commit into from
Jul 9, 2021

Conversation

weaverryan
Copy link
Collaborator

I noticed a minor issue with the section tags. Take the following situation:

h1 Foo
=====

h2 Bar
------

h3 Baz
~~~~~

h2 Bazzles
-----------

The h2 Bazzles should still be a child inside the h1 Foo section. But the HTML markup closes and puts the h2 into a sibling div of the h1.

This includes a failing test case to show the behavior - but I'm not sure where this is controlled in the code.

Cheers!

@greg0ire greg0ire changed the base branch from master to 1.0 November 21, 2020 13:32
@greg0ire greg0ire changed the base branch from 1.0 to 1.0.x November 21, 2020 15:03
@greg0ire greg0ire changed the base branch from 1.0.x to 0.4.x July 9, 2021 18:26
@greg0ire greg0ire force-pushed the over-closing-nesting-issue branch from f7d9fff to 36b5bf5 Compare July 9, 2021 18:26
@greg0ire greg0ire added the Help Wanted Extra attention is needed label Jul 9, 2021
@greg0ire
Copy link
Member

greg0ire commented Jul 9, 2021

@weaverryan I'm super late to the party but I think it is here:

if ($this->lastTitleNode !== null) {
// current level is less than previous so we need to end all open sections
if ($node->getLevel() < $this->lastTitleNode->getLevel()) {
foreach ($this->openTitleNodes as $titleNode) {
$this->endOpenSection($titleNode);
}
// same level as the last so just close the last open section
} elseif ($node->getLevel() === $this->lastTitleNode->getLevel()) {
$this->endOpenSection($this->lastTitleNode);
}
}

@greg0ire
Copy link
Member

greg0ire commented Jul 9, 2021

we need to end all open sections

No we don't! 😛

@greg0ire greg0ire force-pushed the over-closing-nesting-issue branch 2 times, most recently from 788e424 to 890d05b Compare July 9, 2021 19:24
@greg0ire greg0ire requested review from wouterj and removed request for wouterj July 9, 2021 19:24
@greg0ire
Copy link
Member

greg0ire commented Jul 9, 2021

I fixed the test but broke other tests… whack-a-mole!

@greg0ire greg0ire force-pushed the over-closing-nesting-issue branch from 890d05b to 56916b8 Compare July 9, 2021 19:36
@greg0ire greg0ire requested a review from wouterj July 9, 2021 19:38
@greg0ire
Copy link
Member

greg0ire commented Jul 9, 2021

@weaverryan @wouterj please review carefully, it looks super suspicious to me that I had to edit these extra test files.

Just because there is a new chapter does not mean this is a new book.
@greg0ire greg0ire force-pushed the over-closing-nesting-issue branch from 56916b8 to 451cddf Compare July 9, 2021 19:48
Copy link
Collaborator

@wouterj wouterj left a comment

Choose a reason for hiding this comment

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

Looks good!

Don't worry about having to edit other tests if you make a change: It's common in this package that the test files are copy-pasted from the output. E.g. the "titles" test is testing the h1, h2, etc. elements, the rest was just copy-pasted from the output and not looked at. In other words: tests may contain completely bad expectations

@greg0ire greg0ire merged commit b692368 into doctrine:0.4.x Jul 9, 2021
@greg0ire greg0ire added this to the 0.4.1 milestone Jul 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Help Wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants