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

[chore] Improve check/fix make targets #787

Merged

Conversation

joaopgrassi
Copy link
Member

@joaopgrassi joaopgrassi commented Feb 29, 2024

Changes

I often run into a problem where I modify a markdown and forget to update the TOC, and then the PR fails. My flow is:

  • Make changes
  • Run make check
  • Fix things, commit, push

But I noticed that make check does not include the TOC target which makes it not so useful.

It's unfortunate, but today, the toc library does not offer a "check" or "dry-run" feature (there's a PR open jonschlinkert/markdown-toc#148), so the only way to check is to execute it, and verify if there's any pending changes using git, which is exactly what we do in CI today.

I propose that we add the same thing to make check so at least I know locally that I'm missing something and need to commit. I also added markdown-toc to fix even though it will already be fixed with make check. I know it isn't great, but at least me personally, prefer this over waiting for a PR to fail and then having to push again.

Note: Also I found out that prettier is formatting the CONTRIBUTING.md with - bullet points, where markdown-toc uses * by default. So I also changed that and that's why all the modified files (sorry)

Merge requirement checklist

@joaopgrassi joaopgrassi added editorial Editorial changes only (typos, changelog, ...). No content-related changes of any kind. tooling Regarding build, workflows, build-tools, ... labels Feb 29, 2024
@joaopgrassi joaopgrassi requested review from a team February 29, 2024 13:17
@joaopgrassi joaopgrassi requested a review from a team February 29, 2024 13:17
Copy link
Member

@ChrsMark ChrsMark left a comment

Choose a reason for hiding this comment

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

LGTM

@joaopgrassi joaopgrassi merged commit 0e945f1 into open-telemetry:main Mar 6, 2024
10 checks passed
@joaopgrassi joaopgrassi deleted the check-improvements branch March 6, 2024 10:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
editorial Editorial changes only (typos, changelog, ...). No content-related changes of any kind. tooling Regarding build, workflows, build-tools, ...
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants