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

Lint markdown #668

Merged
merged 12 commits into from
Apr 25, 2022
Merged

Lint markdown #668

merged 12 commits into from
Apr 25, 2022

Conversation

wolf99
Copy link
Contributor

@wolf99 wolf99 commented Apr 15, 2022

Making this change with multiple commits to make it easier to revert any parts that people don't like 🙂
I have some questions before making later commits.
In a final commit, I have introduced a GitHub workflow that adds a markdown lint step so that the linting can be easily maintained.

This PR will probably cause merge conflicts with #237, #551, and #607

README.md Outdated
Comment on lines 88 to 90
<!-- this explicit anchor should stay stable so that external docs can link here -->
<a name="linux-install-instructions"></a>
Copy link
Contributor Author

@wolf99 wolf99 Apr 15, 2022

Choose a reason for hiding this comment

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

Is this actually linked from external docs? (@vtbassmatt maybe?)

If so, is there a commitment not to break this specific link for some reason?

Docs (anywhere) can link to headings in markdown using the url#heading syntax, as in docs/wsl.md in this commit. There isn't usually any need to add additional, explicit anchors for markdown headings. There are linting rules against: HTML in markdown; having content in lines immediately before or after a heading.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's linked from at least the GitHub docs on the topic (you may have to click over to the Linux tab) and possibly other vendors as well.

Copy link
Contributor Author

@wolf99 wolf99 Apr 18, 2022

Choose a reason for hiding this comment

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

Thanks! I'll experiment with a few options for solving this in a lint-friendly way then.

Are these docs rendered only on GitHub or also in other locations that I should test the results against?

Copy link
Contributor

Choose a reason for hiding this comment

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

If you mean the docs in this repo, no – we don't render them anyplace else.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Turns out GitHub Flavored Markdown does not support custom heading links in any form.
So I've ended up simply adding an "ignore" for the specific linting rule for this anchor (in 8cf643a)

@wolf99 wolf99 marked this pull request as draft April 15, 2022 22:10
@wolf99
Copy link
Contributor Author

wolf99 commented Apr 16, 2022

Two more general issues remain:

  1. Both docs/environment.md and docs/configuration.md contain many multiple headings with the same content.

    The first contains many headings of:

    • "Example"
    • "Windows"
    • "macOS/Linux"

    While the latter contains many headings of "Example"

    There are some options to resolve this:

    1. Restructure these documents so as to avoid this heading pattern
    2. Add configuration to markdown lint CI to ignore these forever (this option is currently implemented in the workflow introduced in this PR)

    Which is preferred?

  2. There appears to not be a uniform approach to line length. It looks like some files break on a given character column length (something < 80), while others break at the end of sentence.

    My own opinion is that the latter is better for readability and more in the spirit of markdown itself, but as with most things in programming it is more important that any approach be uniform and I am happy to implement any approach that is preferred.

    The linting workflow introduced in this PR is currently configured to ignore line length.

    Is there a preference?

@wolf99 wolf99 marked this pull request as ready for review April 16, 2022 16:53
Copy link
Contributor

@ldennington ldennington left a comment

Choose a reason for hiding this comment

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

Thanks for working to make our docs better! ✏️

CONTRIBUTING.md Outdated
This helps us coordinate and reduce duplication.
0. Once we've had some discussion, you're ready to code!
2. Once we've had some discussion, you're ready to code!
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we able to add an exception instead of making this change? We tend to use the 1/0 formatting on purpose, as it makes it easier to add new items in lists (i.e. we don't have to go change all the other numbers when we put something new in the middle).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've always felt this reasoning was overrated; if someone is making changes to a document anyway, then it's not hard to change a handful of list numbers as well (for relatively short lists). It's can also get a little confusing when working with sub-lists and sub-sub lists, when they are all the same number.

The benefit of proper numbering is that the markdown provides the same information in raw form as in rendered form.

Nevertheless, I have updated the docs to use the form of repeated 1., as this form is accepted by the linter (repeated 0. also works but means that lists start at 0 rather than 1), in commit 1b24d82

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you!


For answers to common questions about this code of conduct, see
https://www.contributor-covenant.org/faq
[homepage]: https://www.contributor-covenant.org
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: maybe cc-homepage or cc_homepage?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call!
Added in 7f3f110

@@ -65,18 +65,18 @@ The Access and Refresh Tokens will be stored against the username and the userna

# On-Premise Bitbucket

On-premise Bitbucket, more correctly known as Bitbucket Server or Bitbucket DC, has a number of differences compared to the cloud instance of Bitbucket, https://bitbucket.org.
On-premise Bitbucket, more correctly known as Bitbucket Server or Bitbucket DC, has a number of differences compared to the cloud instance of Bitbucket, [bitbucket.org](https://bitbucket.org).
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we have short names for links in this document similar to those added in CODE_OF_CONDUCT.md?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes! 🙂
I will work on doing this for all markdown files.

At the same time I will replace aka.ms links that refer to other docs within this repo with relative links, so as to:

  1. Remove dependency on MS short link service
  2. Allow links to work offline.

Please let me know if this latter part is not wanted.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am ok with the latter part, but I think @mjcheetham may want to weigh in on this.

1. No support for OAuth device authorization (necessary for machines without web browser) https://gitlab.com/gitlab-org/gitlab/-/issues/332682
2. Only domains with prefix `gitlab.` are recognised as GitLab remotes https://gitlab.com/gitlab-org/gitlab/-/issues/349464
3. Username/password authentication is suggested even if disabled on server https://gitlab.com/gitlab-org/gitlab/-/issues/349463
1. No support for OAuth device authorization (necessary for machines without web browser): [GitLab issue 332682](https://gitlab.com/gitlab-org/gitlab/-/issues/332682)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we have short names for the links in this document similar to those added in CODE_OF_CONDUCT.md?

@ldennington
Copy link
Contributor

ldennington commented Apr 18, 2022

Two more general issues remain:

  1. Both docs/environment.md and docs/configuration.md contain many multiple headings with the same content.
    The first contains many headings of:

    • "Example"
    • "Windows"
    • "macOS/Linux"

    While the latter contains many headings of "Example"
    There are some options to resolve this:

    1. Restructure these documents so as to avoid this heading pattern
    2. Add configuration to markdown lint CI to ignore these forever (this option is currently implemented in the workflow introduced in this PR)

    Which is preferred?

I think it is fine to ignore in this case. I don't think having multiple of the same heading in one file is a huge deal.

  1. There appears to not be a uniform approach to line length. It looks like some files break on a given character column length (something < 80), while others break at the end of sentence.
    My own opinion is that the latter is better for readability and more in the spirit of markdown itself, but as with most things in programming it is more important that any approach be uniform and I am happy to implement any approach that is preferred.
    The linting workflow introduced in this PR is currently configured to ignore line length.
    Is there a preference?

My preference would be to break on a character column link. If we decide to do this, I'm good with doing < 80.

@wolf99 wolf99 mentioned this pull request Apr 19, 2022
@wolf99
Copy link
Contributor Author

wolf99 commented Apr 23, 2022

Thank you for the review @ldennington !

There are two things remaining in progress

  1. Line lengths
  2. Reference style links

I will continue to work on these, but perhaps this can be merged as it currently is, with those as separate PRs?

The reason being it is already a large change and I am aware of other PRs that also have doc changes, I wouldn't like this PR to block these changes cause too many merge conflicts to those or vice-versa. Both of the above changes will be a lot of change across many files.

@wolf99
Copy link
Contributor Author

wolf99 commented Apr 23, 2022

Rebased and fixed merge conflicts

@ldennington
Copy link
Contributor

Thank you for the review @ldennington !

There are two things remaining in progress

  1. Line lengths
  2. Reference style links

I will continue to work on these, but perhaps this can be merged as it currently is, with those as separate PRs?

The reason being it is already a large change and I am aware of other PRs that also have doc changes, I wouldn't like this PR to block these changes cause too many merge conflicts to those or vice-versa. Both of the above changes will be a lot of change across many files.

I'm fine with this!

Thank you for the review @ldennington !

There are two things remaining in progress

  1. Line lengths
  2. Reference style links

I will continue to work on these, but perhaps this can be merged as it currently is, with those as separate PRs?

The reason being it is already a large change and I am aware of other PRs that also have doc changes, I wouldn't like this PR to block these changes cause too many merge conflicts to those or vice-versa. Both of the above changes will be a lot of change across many files.

Sounds great! I just did another review, and it all looks good. I will proceed with the merge.

@ldennington ldennington merged commit 86714ff into git-ecosystem:main Apr 25, 2022
@wolf99 wolf99 deleted the lint-markdown branch April 26, 2022 09:26
@ldennington ldennington mentioned this pull request Jun 15, 2022
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.

4 participants