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

Add support for indented link references on excerpt #5212

Merged
merged 1 commit into from Oct 5, 2016

Conversation

Projects
None yet
6 participants
@eloyesp
Contributor

eloyesp commented Aug 8, 2016

Excerpt link reference extraction is missing all the indented references
at the bottom of the page. Markdown specify that those can be indented up
to three spaces.

I've also added a title to the link in the test.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Aug 11, 2016

Member

Why would a reference be indented like this? Also, can you add tests for this instead of modifying pre-existing ones? Thanks!

Member

parkr commented Aug 11, 2016

Why would a reference be indented like this? Also, can you add tests for this instead of modifying pre-existing ones? Thanks!

@parkr parkr added bug fix labels Aug 11, 2016

@eloyesp

This comment has been minimized.

Show comment
Hide comment
@eloyesp

eloyesp Aug 16, 2016

Contributor

@parkr

Why would a reference be indented like this?

I think that it makes it more readable, it seems that stack overflow thinks the same.

Also, can you add tests for this instead of modifying pre-existing ones?

I think that adding more tests could make the test suite harder to read, but I've changed that as requested.

Contributor

eloyesp commented Aug 16, 2016

@parkr

Why would a reference be indented like this?

I think that it makes it more readable, it seems that stack overflow thinks the same.

Also, can you add tests for this instead of modifying pre-existing ones?

I think that adding more tests could make the test suite harder to read, but I've changed that as requested.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Aug 16, 2016

Contributor

I think that adding more tests could make the test suite harder to read, but I've changed that as requested.

The test suite isn't there for you to read, it's there for us to make sure what you do works.

Contributor

envygeeks commented Aug 16, 2016

I think that adding more tests could make the test suite harder to read, but I've changed that as requested.

The test suite isn't there for you to read, it's there for us to make sure what you do works.

@eloyesp

This comment has been minimized.

Show comment
Hide comment
@eloyesp

eloyesp Aug 31, 2016

Contributor

Any news about this PR? The failing tests are not related to the introduced changes.

Contributor

eloyesp commented Aug 31, 2016

Any news about this PR? The failing tests are not related to the introduced changes.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Aug 31, 2016

Contributor

@eloyesp I restarted the tests and they failed a second time, can you double check your changes didn't cause the failure please!

Contributor

envygeeks commented Aug 31, 2016

@eloyesp I restarted the tests and they failed a second time, can you double check your changes didn't cause the failure please!

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Aug 31, 2016

Member

Failure is because you added a post to the test site, so the number of generated posts is different from what the test suite expects.

Member

pathawks commented Aug 31, 2016

Failure is because you added a post to the test site, so the number of generated posts is different from what the test suite expects.

@eloyesp

This comment has been minimized.

Show comment
Hide comment
@eloyesp

eloyesp Sep 2, 2016

Contributor

thanks @pathawks, It's fixed now.

Contributor

eloyesp commented Sep 2, 2016

thanks @pathawks, It's fixed now.

Show outdated Hide outdated test/source/_posts/2016-08-16-indented-link-references.markdown
---
---
This is the first paragraph. It [have][link_0] [lots][link_1] [of][link_2]

This comment has been minimized.

@Strangehill

Strangehill Sep 2, 2016

Contributor

'has', not 'have'

@Strangehill

Strangehill Sep 2, 2016

Contributor

'has', not 'have'

Show outdated Hide outdated test/source/_posts/2016-08-16-indented-link-references.markdown
This is the first paragraph. It [have][link_0] [lots][link_1] [of][link_2]
[links][link_3].
This is the second paragraph. It have a sample code that should not be

This comment has been minimized.

@Strangehill

Strangehill Sep 2, 2016

Contributor

'has sample', not 'have a sample'

@Strangehill

Strangehill Sep 2, 2016

Contributor

'has sample', not 'have a sample'

This comment has been minimized.

@envygeeks

envygeeks Sep 2, 2016

Contributor

That matters none, it's for a test, not for user consumption.

@envygeeks

envygeeks Sep 2, 2016

Contributor

That matters none, it's for a test, not for user consumption.

This comment has been minimized.

@eloyesp

eloyesp Sep 2, 2016

Contributor

@Strangehill thanks anyway, I should work on my English :)

@eloyesp

eloyesp Sep 2, 2016

Contributor

@Strangehill thanks anyway, I should work on my English :)

This comment has been minimized.

@Strangehill

Strangehill Sep 2, 2016

Contributor

It's just nitpicking but since it's not merged yet I didn't see the harm --- and I figured easy to read tests acts as their own sort of documentation.

@Strangehill

Strangehill Sep 2, 2016

Contributor

It's just nitpicking but since it's not merged yet I didn't see the harm --- and I figured easy to read tests acts as their own sort of documentation.

Add support for indented link references on excerpt
Excerpt link reference extraction is missing all the indented references
at the bottom of the page. Markdown specify that those can be indented up
to three spaces.
@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Sep 9, 2016

Member

LGTM.

Member

parkr commented Sep 9, 2016

LGTM.

@parkr parkr added this to the 3.3 milestone Sep 9, 2016

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Sep 9, 2016

Contributor

LGTM.

Contributor

envygeeks commented Sep 9, 2016

LGTM.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Oct 5, 2016

Member

@jekyllbot: merge +minor

Member

parkr commented Oct 5, 2016

@jekyllbot: merge +minor

@jekyllbot jekyllbot merged commit 22a7714 into jekyll:master Oct 5, 2016

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jekyll/lgtm Approved by @parkr and @envygeeks.

jekyllbot added a commit that referenced this pull request Oct 5, 2016

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