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

🏗📖 Cap line length of Markdown files #26365

Closed
wants to merge 2 commits into from
Closed

🏗📖 Cap line length of Markdown files #26365

wants to merge 2 commits into from

Conversation

DerekNonGeneric
Copy link

I am in the process of adding Markdown linting to the amp.dev repo (ampproject/amp.dev#3346). During the last wg-outreach meeting I was informed that the same mechanism used in the amphtml repo should be used.

Nice to have: capping the line-length of Markdown files to 80 chars. It would greatly assist with revising proposed changes when looking at the diffs of typo/grammar PRs in GitHub. For example, look at line 75 of this diff. It's difficult to find what was changed.

Problem: running npx gulp prettify --fix does not auto-format the max line-length of Markdown files. They should theoretically be maxing 80 chars if this task inherits the default print-width of prettier.

Solution: explicitly specify the line-length of Markdown files in .prettierrc.

/cc @rsimha

@amp-owners-bot
Copy link

Hey @estherkim, these files were changed:

  • build-system/tasks/e2e/README.md

Hey @alanorozco, these files were changed:

  • extensions/amp-3q-player/amp-3q-player.md
  • extensions/amp-brid-player/amp-brid-player.md
  • extensions/amp-connatix-player/amp-connatix-player.md
  • extensions/amp-delight-player/amp-delight-player.md
  • extensions/amp-kaltura-player/amp-kaltura-player.md
  • extensions/amp-minute-media-player/amp-minute-media-player.md
  • extensions/amp-nexxtv-player/amp-nexxtv-player.md
  • extensions/amp-o2-player/amp-o2-player.md
  • extensions/amp-ooyala-player/amp-ooyala-player.md
  • extensions/amp-powr-player/amp-powr-player.md
  • extensions/amp-reach-player/amp-reach-player.md
  • extensions/amp-redbull-player/amp-redbull-player.md
  • extensions/amp-springboard-player/amp-springboard-player.md
  • extensions/amp-viqeo-player/amp-viqeo-player.md
  • extensions/amp-wistia-player/amp-wistia-player.md

Hey @wassgha, these files were changed:

  • extensions/amp-3q-player/amp-3q-player.md
  • extensions/amp-brid-player/amp-brid-player.md
  • extensions/amp-connatix-player/amp-connatix-player.md
  • extensions/amp-delight-player/amp-delight-player.md
  • extensions/amp-kaltura-player/amp-kaltura-player.md
  • extensions/amp-minute-media-player/amp-minute-media-player.md
  • extensions/amp-nexxtv-player/amp-nexxtv-player.md
  • extensions/amp-o2-player/amp-o2-player.md
  • extensions/amp-ooyala-player/amp-ooyala-player.md
  • extensions/amp-powr-player/amp-powr-player.md
  • extensions/amp-reach-player/amp-reach-player.md
  • extensions/amp-redbull-player/amp-redbull-player.md
  • extensions/amp-springboard-player/amp-springboard-player.md
  • extensions/amp-viqeo-player/amp-viqeo-player.md
  • extensions/amp-wistia-player/amp-wistia-player.md

Hey @gmajoulet, these files were changed:

  • extensions/amp-story/0.1/media-pool.md
  • extensions/amp-story/1.0/_locales/README.md
  • extensions/amp-story/amp-story-ads.md
  • extensions/amp-story/amp-story-analytics.md
  • extensions/amp-story/amp-story-auto-ads.md
  • extensions/amp-story/amp-story-bookend.md
  • extensions/amp-story/amp-story-cta-layer.md
  • extensions/amp-story/amp-story-grid-layer.md
  • extensions/amp-story/amp-story-page-attachment.md
  • extensions/amp-story/amp-story-page.md
  • extensions/amp-story/amp-story.md

Hey @newmuis, these files were changed:

  • extensions/amp-story/0.1/media-pool.md
  • extensions/amp-story/1.0/_locales/README.md
  • extensions/amp-story/amp-story-ads.md
  • extensions/amp-story/amp-story-analytics.md
  • extensions/amp-story/amp-story-auto-ads.md
  • extensions/amp-story/amp-story-bookend.md
  • extensions/amp-story/amp-story-cta-layer.md
  • extensions/amp-story/amp-story-grid-layer.md
  • extensions/amp-story/amp-story-page-attachment.md
  • extensions/amp-story/amp-story-page.md
  • extensions/amp-story/amp-story.md

@rsimha
Copy link
Contributor

rsimha commented Jan 16, 2020

IIRC, we didn't initially enforce line width limits for .md files because of the sheer number of lines of diffs they would generate.

I've added @mrjoro and @CrystalOnScript to comment on whether this is desirable.

@mrjoro
Copy link
Member

mrjoro commented Jan 16, 2020

I'd lean towards not enforcing this line length limit. md files often get modified in the web ui and there isn't a good way of knowing when a line goes past 80 chars.

@DerekNonGeneric
Copy link
Author

I see what you're getting at. #21212 already had a 138,754 line diff. Including this would've been a bit much. I guess this can be seen as a continuation of that effort.

From a workflow perspective, VS Code's editor.formatOnSave option, which is already enabled in this project, makes composing in Markdown files while still following this rule so much nicer.

@DerekNonGeneric
Copy link
Author

/to @mrjoro

md files often get modified in the web ui and there isn't a good way of knowing when a line goes past 80 chars.

Would the CI check be insufficient? Maybe the CI can even auto-format the files and recommit? I'm not sure if that is already happening.

@rsimha
Copy link
Contributor

rsimha commented Jan 16, 2020

I see what you're getting at. #21212 already had a 138,754 line diff. Including this would've been a bit much. I guess this can be seen as a continuation of that effort.

Maybe the CI can even auto-format the files and recommit? I'm not sure if that is already happening.

This doesn't happen, and is not something we plan on supporting.

I'd lean towards not enforcing this line length limit. md files often get modified in the web ui and there isn't a good way of knowing when a line goes past 80 chars.

I think this is the crux of the problem. We don't want to make it even more onerous for folks to edit .md files via GitHub's web UI.

@DerekNonGeneric
Copy link
Author

DerekNonGeneric commented Jan 16, 2020

We don't want to make it even more onerous for folks to edit .md files via GitHub's web UI.

It's a shame the GitHub web UI doesn't infer this based on a repo's .editorconfig file (or allow a user to set it themselves). A potential workaround exists via userscript until GitHub includes this feature themselves. There is some prior art with the GitHub code guides userscript.

@DerekNonGeneric DerekNonGeneric changed the title 🏗✨ Cap line length of Markdown files 🏗📖 Cap line length of Markdown files Jan 17, 2020
@mrjoro
Copy link
Member

mrjoro commented Jun 11, 2020

Is this PR still active?

@DerekNonGeneric DerekNonGeneric marked this pull request as draft June 11, 2020 19:37
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@DerekNonGeneric
Copy link
Author

I guess I will close this. I still stand by it, but it seems like this project doesn't want to do this. I wonder if there was a change of mind since I opened this? Please do tell. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants