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

End footnote definition with one blank line. #21

Merged
merged 3 commits into from Jun 21, 2017

Conversation

Projects
None yet
6 participants
@raphlinus
Copy link
Owner

raphlinus commented Apr 10, 2016

According to the linked issue, a footnote definition should end with a
blank line. This is similar to the rule for lists, which end with two
blank lines. The code previously required two blank lines in both
cases, this patch changes it to just one for footnote definitions.

Fixes issue 20.

End footnote definition with one blank line.
According to the linked issue, a footnote definition should end with a
blank line. This is similar to the rule for lists, which end with two
blank lines. The code previously required two blank lines in both
cases, this patch changes it to just one for footnote definitions.

Fixes issue 20.
@googlebot

This comment has been minimized.

Copy link

googlebot commented Apr 10, 2016

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

raphlinus added some commits Apr 10, 2016

End footnote definition with one blank line.
According to the linked issue, a footnote definition should end with a
blank line. This is similar to the rule for lists, which end with two
blank lines. The code previously required two blank lines in both
cases, this patch changes it to just one for footnote definitions.

Fixes issue 20.
@googlebot

This comment has been minimized.

Copy link

googlebot commented Apr 10, 2016

CLAs look good, thanks!

@raphlinus

This comment has been minimized.

Copy link
Owner Author

raphlinus commented Apr 10, 2016

Fixes issue #20, see more discussion there.

@notriddle

This comment has been minimized.

Copy link
Contributor

notriddle commented Apr 10, 2016

If this is going to be merged, specs/footnote.txt needs to be updated.

@GuillaumeGomez

This comment has been minimized.

Copy link
Contributor

GuillaumeGomez commented Mar 29, 2017

I need this to be merged. Do you know when this we'll done?

@raphlinus

This comment has been minimized.

Copy link
Owner Author

raphlinus commented Mar 29, 2017

In order to merge this, I need more confidence that it's compliant with the spec, which I currently don't have. Otherwise, if you need non-spec-compliant behavior for compatibility, then divergence from the spec needs to be a flag that gets set.

@GuillaumeGomez

This comment has been minimized.

Copy link
Contributor

GuillaumeGomez commented Mar 29, 2017

Seems like it is.

@raphlinus

This comment has been minimized.

Copy link
Owner Author

raphlinus commented Mar 29, 2017

Ah, ok. I'll play a bit with whether this PR actually does the right thing, and if so I'll merge it relatively soon.

@GuillaumeGomez

This comment has been minimized.

Copy link
Contributor

GuillaumeGomez commented Mar 29, 2017

Great, thanks a lot!

PS: pulldown-cmark is now the markdown parser used in rustdoc (congrats!).

@ScottAbbey

This comment has been minimized.

Copy link
Collaborator

ScottAbbey commented Mar 30, 2017

Seems like it is.

I don't think this shows anything in favor or against. That tool has no support for footnotes so it interprets [^footnote]: Thing as a link to "Thing".

It appears as though the footnote spec in this repository treats them more like link references than list items. This is probably not right because link references can only contain a small number of specific items, while a footnote seems like it should be a container block.

I also just left a comment at #20 (comment).

bors added a commit to rust-lang/rust that referenced this pull request Apr 2, 2017

Auto merge of #40919 - GuillaumeGomez:fix-new-rustdoc, r=frewsxcv,ste…
…veklabnik

Add support for image, rules and footnotes

Part of #40912.

r? @rust-lang/docs

PS: the footnotes are waiting for raphlinus/pulldown-cmark#21 to be merged to be fully working.

@steveklabnik steveklabnik referenced this pull request Apr 5, 2017

Closed

Tracking issue for hoedown -> pulldown regressions #40912

13 of 13 tasks complete
@raphlinus

This comment has been minimized.

Copy link
Owner Author

raphlinus commented Apr 20, 2017

Based on discussion in https://internals.rust-lang.org/t/what-to-do-about-pulldown-and-commonmark/5115 I am inclined to merge this. Any final comments?

Thanks to everybody who's weighed in here, and sorry for not taking action on it sooner.

@GuillaumeGomez

This comment has been minimized.

Copy link
Contributor

GuillaumeGomez commented Apr 20, 2017

Finally! \o/

@critiqjo

This comment has been minimized.

Copy link
Contributor

critiqjo commented Apr 21, 2017

Since this would be a "breaking change" anyway, may I suggest breaking a little further and disallowing blocks inside footnote definition (if it is easy enough)? For example:

a [^b].

[^b]: * c
* d

Should produce either:

<p>a <sup class="footnote-reference"><a href="#b">1</a></sup>.</p>
<div class="footnote-definition" id="b"><sup class="footnote-definition-label">1</sup>
* c
* d
</div>

or

<p>a <sup class="footnote-reference"><a href="#b">1</a></sup>.</p>
<div class="footnote-definition" id="b"><sup class="footnote-definition-label">1</sup>
* c
</div>
<ul>
<li>d</li>
</ul>

So that this would just be a baseline implementation of footnotes. And further changes based on whichever spec that we choose to adopt will not "break" much. Since pulldown-cmark is now being used by rustdoc, implementation-specific features should be kept as thin as possible, in my opinion.

@raphlinus raphlinus merged commit 13918e7 into master Jun 21, 2017

1 check passed

cla/google All necessary CLAs are signed
@GuillaumeGomez

This comment has been minimized.

Copy link
Contributor

GuillaumeGomez commented Jun 21, 2017

Thanks!

@raphlinus

This comment has been minimized.

Copy link
Owner Author

raphlinus commented Jun 21, 2017

I'm merging this because it seems like an overall improvement. The suggestion by critiqjo seems valid, but I haven't had time to research it in detail. I'd happily take a patch for it as a further refinement, as long as there's evidence it won't cause regressions, etc.

This is released as v0.0.15.

@raphlinus raphlinus deleted the issue20 branch Jun 21, 2017

@ScottAbbey

This comment has been minimized.

Copy link
Collaborator

ScottAbbey commented Jul 13, 2017

This broke several footnotes spec tests:

failures:
    spec_test_3
    spec_test_4
    spec_test_5
    spec_test_7

test result: FAILED. 4 passed; 4 failed; 0 ignored; 0 measured

It's important to pass all these since they are basically the only definition of this particular footnote spec. I haven't looked at them, but I am guessing the tests need to be updated.

raphlinus added a commit that referenced this pull request Aug 9, 2017

Fix tests broken by footnote change
The footnote change (issue #21) broke some tests. In some cases (just
the number of newlines) the tests needed to be updated, but in others
(allowing complex block structure) the code needed to be fixed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.