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
Merged

End footnote definition with one blank line. #21

merged 3 commits into from
Jun 21, 2017

Conversation

raphlinus
Copy link
Collaborator

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.

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
Copy link

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.

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
Copy link

CLAs look good, thanks!

@raphlinus
Copy link
Collaborator Author

Fixes issue #20, see more discussion there.

@notriddle
Copy link
Collaborator

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

@GuillaumeGomez
Copy link
Contributor

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

@raphlinus
Copy link
Collaborator Author

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
Copy link
Contributor

Seems like it is.

@raphlinus
Copy link
Collaborator Author

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
Copy link
Contributor

Great, thanks a lot!

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

@ScottAbbey
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
…veklabnik

Add support for image, rules and footnotes

Part of #40912.

r? @rust-lang/docs

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

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
Copy link
Contributor

Finally! \o/

@critiqjo
Copy link
Contributor

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
@GuillaumeGomez
Copy link
Contributor

Thanks!

@raphlinus
Copy link
Collaborator 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 June 21, 2017 14:56
@ScottAbbey
Copy link
Collaborator

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
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants