Skip to content

Fix bullet titles and add anchors #240

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

Merged
merged 3 commits into from
Oct 24, 2019

Conversation

harding
Copy link
Collaborator

@harding harding commented Oct 21, 2019

This PR makes the newsletter content pass the tests described here and then adds those tests to the makefile. By putting those bullet titles all on one line, they're automatically turned into anchors by a plugin added in a previous PR.

In a final commit, several anchors are manually added. Content in the not-yet-added topics branches refers to those anchors.

Copy link
Contributor

@jnewbery jnewbery 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 @harding . A few nits inline.

I've tested that the first and third commits only changes whitepace and ids, both in the source md files and the generated site. I've also tested that the new makefile test fails on master and passes on this branch.

@@ -21,6 +21,10 @@ test-before-build:
! git --no-pager grep -L "^slug: " _posts
## Check that all slugs are unique
! git --no-pager grep -h "^slug: " _posts | sort | uniq -d | grep .
## Check for things that should probably all be on one line
@ ## Note: double $$ in a makefile produces a single literal $
! git --no-pager grep -- '^ *- \*\*[^\*]*$$'
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think the backslash is necessary in the square brackets:

→ echo '*' | grep [*]; echo $?
*
0
→ echo '*' | grep [^*]; echo $?
1

I've verified that removing the \ yields the same result when run against master.

@@ -325,6 +331,7 @@ recommended) using Tor---which can provide other benefits---but enabling
encryption by default could help protect a larger number of users from
eavesdropping by their ISPs.

{:#untrackable-auth}
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason not to use 'countersign' as the anchor here?

@@ -205,6 +205,7 @@ it to write a good description, and I doubt non-LN devs care -->{% endcomment %}
- [LND #1981][] ensures that LND doesn't leak information about any of
its peers that aren't advertising themselves as public nodes.

{:#lnd-1535-1512}
Copy link
Contributor

Choose a reason for hiding this comment

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

This splits the next bullet into its own list. Would it be better to just make [LND #1535] the id for the bullet?

@@ -38,10 +38,10 @@ changes to popular Bitcoin infrastructure projects.
addresses, tracking when they've been paid, and finding or
deriving the particular private keys necessary for spending.

- June 6th: discussion of the [consensus cleanup soft fork][xs
- <span id="cleanup-discussion" markdown="1">June 6th: discussion of the [consensus cleanup soft fork][xs
Copy link
Contributor

Choose a reason for hiding this comment

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

How about:

    - 
      {:#cleanup-discussion}
      June 6th: discussion of the [consensus cleanup soft fork][xs

Same for assume-utxo-demo below.

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW I couldn't get what John outlined to both work and pass the "things that should be on one line" check at the same.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@bitschmidty yeah, I had the same problems when trying different variations. However, FYI, Block Attribute Lists (BALs) can go either before or after the block you want them to act on, so I was able to put it after the first paragraph of that bullet:

    - June 6th: discussion of the [consensus cleanup soft fork][xs
      cleanup sf] included its interaction with [bip-taproot][], whether
      parts of it should be dropped, and whether anything should be
      added.  ([More background][bg consensus cleanup])
      {:#cleanup-discussion}

Copy link
Contributor

@bitschmidty bitschmidty left a comment

Choose a reason for hiding this comment

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

lightly tested ACK

@harding harding force-pushed the 2019-10-add-anchors branch from 1d2750d to ede049d Compare October 22, 2019 23:17
@harding
Copy link
Collaborator Author

harding commented Oct 22, 2019

Forced pushed edits for @jnewbery feedback (thanks!)

@@ -205,7 +205,8 @@ it to write a good description, and I doubt non-LN devs care -->{% endcomment %}
- [LND #1981][] ensures that LND doesn't leak information about any of
its peers that aren't advertising themselves as public nodes.

- LND [#1535][LND #1535] and [#1512][LND #1512] adds the server-side
- {:#lnd-1535-1512}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is still moving this one bullet point into its own <ul>. Can you move this Block Attribute List to after the text?

Copy link
Contributor

Choose a reason for hiding this comment

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

actually, I've just done this myself and pushed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think that edit is fine, but are you sure you were testing the updated branch? On my branch (ede049d, without your change), I don't see an extraneous <ul>:

  </li>
  <li id="lnd-1535-1512">
    <p>LND <a href="https://github.com/lightningnetwork/lnd/issues/1535">#1535</a> and <a href="https://github.com/lightningnetwork/lnd/issues/1512">#1512</a> adds the server-side

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, looking at the GitHub forced push, it looks like you merged the old version of this with your one change.

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.

3 participants