Skip to content

Proofread docs#3040

Merged
TomasVotruba merged 2 commits intorectorphp:masterfrom
greg0ire:proofread-docs
Mar 21, 2020
Merged

Proofread docs#3040
TomasVotruba merged 2 commits intorectorphp:masterfrom
greg0ire:proofread-docs

Conversation

@greg0ire
Copy link
Copy Markdown
Contributor

No description provided.

Comment thread docs/HowItWorks.md Outdated
Comment on lines 7 to 8
- The application finds files in the source code you provide and registered
Rectors - from `--set`, `--config` or local `rector.yaml`
- Then it iterates all found files and applies relevant Rectors to them.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Avoid line breaks as it mall-formats on various dimensions (pc, smart phone mobile...). Let browser handle it.

Copy link
Copy Markdown
Contributor Author

@greg0ire greg0ire Mar 21, 2020

Choose a reason for hiding this comment

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

The line breaks are not taken into account while displaying the rendered html README, this is just for source code and diffs, you can check that by clicking "Display the rich diff"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What is the reason then?

It's very hard to read and add extra work to control spaces everytime case content changes.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The reasons are making diffs easier to read, reducing the risk of conflicts, allow displaying the file side by side with another. Actually I wrote an entire blog post about that: https://dev.to/greg0ire/why-i-keep-pestering-everyone-about-long-lines
Also, it can spare you from having to scroll horizontally when reviewing (there is no wrapping in the editor on Github), and is just easier for humans to read when displayed in an editor and no wrapping occurs.

I would agree that for some lines that are a bit long but not that much, the benefits are not huge though. If you are still not convinced just tell me, and I will just drop that commit. For the other PR, it would be harder though, and the benefits a more visible (the lines are so long there that it is more worth it, also there is some code that really benefits from not having a horizontal scroll).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If you automate it, I don't mind. But I don't want to add extra maintenance cost.

Copy link
Copy Markdown
Contributor Author

@greg0ire greg0ire Mar 21, 2020

Choose a reason for hiding this comment

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

I don't know of a way to automate that, be it for md files php files or any file, so I'll just drop the commit I guess.

Copy link
Copy Markdown
Member

@TomasVotruba TomasVotruba Mar 21, 2020

Choose a reason for hiding this comment

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

FYI, In PHP there is: https://github.com/symplify/coding-standard/blob/master/src/Fixer/LineLength/LineLengthFixer.php

It would me madness to ask developers to do it manually.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good to know, but I don't know any fixer for markdown.

It would me madness to ask developers to do it manually.

I don't know, I feel humans might often be better than robots at this, because they will split a line where it is meaningful to do so. Plus there are tools built in IDEs to make it a bit less manual I think

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's about the 0-entrance cost adoption. If you add CI tool, I don't mind and the change would be accepted.

Copy link
Copy Markdown
Member

@TomasVotruba TomasVotruba Mar 21, 2020

Choose a reason for hiding this comment

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

Also, this PR is self-example of how human suck in these tasks (me in my English :D)

@TomasVotruba
Copy link
Copy Markdown
Member

Thank you 👍

@TomasVotruba TomasVotruba merged commit ae61817 into rectorphp:master Mar 21, 2020
@greg0ire greg0ire deleted the proofread-docs branch March 21, 2020 14:31
TomasVotruba added a commit that referenced this pull request Nov 10, 2022
rectorphp/rector-src@baf8394 Fix var/property usage for RemoveUnusedNonEmptyArrayBeforeForeachRector. (#3040)
TomasVotruba added a commit that referenced this pull request Nov 10, 2022
rectorphp/rector-src@baf8394 Fix var/property usage for RemoveUnusedNonEmptyArrayBeforeForeachRector. (#3040)
TomasVotruba added a commit that referenced this pull request Nov 10, 2022
rectorphp/rector-src@baf8394 Fix var/property usage for RemoveUnusedNonEmptyArrayBeforeForeachRector. (#3040)
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.

2 participants