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

Format documentation for better diffing of #5534 #5545

Closed
wants to merge 2 commits into from
Closed

Format documentation for better diffing of #5534 #5545

wants to merge 2 commits into from

Conversation

David-Else
Copy link
Contributor

This pull request simply formats all the current documentation using the same settings as #5534 . There are absolutely no other changes applied.

I am waiting for a reply from @archseer #5534 (comment) to confirm if he thinks doing this is the best solution to make reviewing easier by making the changes easier to see, but I put in this pull request in advance to save time if the idea is approved.

I have included the .prettierrc file for future reference.

@the-mikedavis I don't expect there to be any problems created by this for the mdBook generation, but maybe you could confirm that?

@tmke8
Copy link

tmke8 commented Jan 16, 2023

Don't you get the best diffs with one sentence per line? (Or, more generally with “semantic line breaks”, so breaking at commas or colons or the start of a new clause.)

@David-Else
Copy link
Contributor Author

@David-Else David-Else closed this Jan 16, 2023
@David-Else David-Else reopened this Jan 16, 2023
@David-Else
Copy link
Contributor Author

David-Else commented Jan 16, 2023

It seems this fix might be preferable to GitHub prose diff mode. You can get the same results with the following, making sure the .prettierrc is in book/src:

.prettierrc

{
  "proseWrap": "always"
}

Note: You must CD into the directory.

npm install -g prettier

cd book/src/
prettier --write "*.md"
cd guides/
prettier --write "*.md"

If it is preferable to taking a pull request.

@David-Else
Copy link
Contributor Author

@thomkeh Thanks for that link. I did't think it would be a good idea in this situation as GitHub has automatic prose diff rendering that takes care of line break issues, and I was asked to format to 80 chars by the-mikedavis.

Semantic line breaks are an interesting idea, but I don't want to take on a whole new work flow, and definitely not back port it to what I have done. I don't think I agree with it either, and have not seen it done anywhere else.

I have looked about and it seems that some big OS docs don't format to 80:

https://raw.githubusercontent.com/reactjs/reactjs.org/main/content/docs/accessibility.md
https://raw.githubusercontent.com/vuejs/docs/main/src/about/community-guide.md
https://raw.githubusercontent.com/microsoft/vscode-docs/main/docs/editor/debugging.md

That would be:

{
  "proseWrap": "never"
}

That was awful to work with before Helix had soft-wrap, but is workable now, if I keep using the PR. Maybe it would work better for GitHub diffs, I am not convinced, but I really don't have the experience to say.

Some big projects do format to 80 chars, and I am sure they use GitHub diffs all day and night to check new pull requests:

https://raw.githubusercontent.com/nodejs/node/main/doc/contributing/api-documentation.md
https://raw.githubusercontent.com/denoland/manual/main/advanced/language_server/overview.md

So I really don't know, it is up to the maintainers.

@the-mikedavis
Copy link
Member

Hmm, we may want to abandon the 80-column formatting change. I like 80 columns but it causes a bunch of formatting churn with the existing docs. It will matter much less once soft-wrapping lands anyways and we'd like to land that PR sooner rather than later. Sorry for the mislead here

@David-Else
Copy link
Contributor Author

David-Else commented Jan 17, 2023

@the-mikedavis Thanks for the update. I am in a bit of a limbo here, I need some feedback on the work I have done on the documentation before continuing, but am told the current diffing is stopping any reviews being done.

Basically Rewrite and refactor all documentation can be "proseWrap": "always" or "proseWrap": "never", there is no way to revert it to the random formatting state it is now. When I was working on it I was using Helix and relying on constant formatting to keep the text on screen, that was before trying the new softwrap PR.

Maybe you could you format the current documentation to "proseWrap": "never" and then I will format my work to the same? Will that solve the problem? I can do it for you in another pull request if you like?

That would result in the current 80 col formatting:

# Installation

We provide pre-built binaries on the
[GitHub Releases page](https://github.com/helix-editor/helix/releases).

[![Packaging status](https://repology.org/badge/vertical-allrepos/helix.svg)](https://repology.org/project/helix/versions)

## OSX

Helix is available in homebrew-core:

```
brew install helix
```

## Linux

### NixOS

A [flake](https://nixos.wiki/wiki/Flakes) containing the package is available in
the project root. The flake can also be used to spin up a reproducible
development shell for working on Helix with `nix develop`.

Flake outputs are cached for each push to master using
[Cachix](https://www.cachix.org/). The flake is configured to automatically make
use of this cache assuming the user accepts the new settings on first use.

If you are using a version of Nix without flakes enabled you can
[install Cachix cli](https://docs.cachix.org/installation); `cachix use helix`
will configure Nix to use cached outputs when possible.

Becoming:

# Installation

We provide pre-built binaries on the [GitHub Releases page](https://github.com/helix-editor/helix/releases).

[![Packaging status](https://repology.org/badge/vertical-allrepos/helix.svg)](https://repology.org/project/helix/versions)

## OSX

Helix is available in homebrew-core:

```
brew install helix
```

## Linux

### NixOS

A [flake](https://nixos.wiki/wiki/Flakes) containing the package is available in the project root. The flake can also be used to spin up a reproducible development shell for working on Helix with `nix develop`.

Flake outputs are cached for each push to master using [Cachix](https://www.cachix.org/). The flake is configured to automatically make use of this cache assuming the user accepts the new settings on first use.

If you are using a version of Nix without flakes enabled you can [install Cachix cli](https://docs.cachix.org/installation); `cachix use helix` will configure Nix to use cached outputs when possible.


@the-mikedavis
Copy link
Member

The problem is with bundling content changes with auto-formatting changes - switching to different auto-formatting settings won't really make a difference.

I could go through the #5543 changes and rewrite them on master without the formatting changes and force-push that to your branch if you'd like. (This would be my way to work through and review the changes as well.)

@David-Else
Copy link
Contributor Author

@the-mikedavis I am quite new to advanced Git and don't really understand what force pushing to my branch in GitHub would mean, would that result with a new commit from you appearing with your new version on my branch? If so, go for it!

You will have a fun job moving all the text back to the old non formatted version, a good test of your Helix keyboard skills :)

  1. Do I need to do anything now (like merge in new changes from upstream master) or just wait to hear something from you over on Rewrite and refactor all documentation #5534 ?
  2. Shall we get what I have done onto the master and then I make a new pull request for the other changes I detail in Rewrite and refactor all documentation #5534 ?

Cheers.

@the-mikedavis
Copy link
Member

Yep it would just be rewritten commit(s) on your branch in that PR. You would still be the author on the commits too. I'll rebase the changes on master so no need to do anything to the branch. I'll try to take a look at this in the next few days.

Shall we get what I have done onto the master and then I make a new pull request for the other changes I detail in #5534?

Yeah we can start with the changes in that branch and make improvements from there once it's merged

@pascalkuthe
Copy link
Member

If I understood this comment #5534 (comment) right this PR can be closed now. Correct?

@David-Else David-Else closed this Feb 1, 2023
@David-Else David-Else deleted the format-documentation branch February 1, 2023 07:46
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