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: add markdownlint to lint commands #9353

Merged
merged 11 commits into from
May 27, 2021
Merged

Conversation

ryanchristo
Copy link
Contributor

@ryanchristo ryanchristo commented May 18, 2021

Description

This pull request adds markdownlint to the lint commands defined in Makefile.

  • make lint will now report issues with markdown files
  • make lint-fix will now attempt to fix issues with markdown files

The configuration for markdownlint was originally copied from tendermint (see markdownlint.yml) and then modified to include additional rules to help limit the scope of this pull request.

  • .markdownlint.json and .markdownlintignore set the configuration
  • docs/basics/app-anatomy.md was updated manually for a missing link
  • the remaining files were updated using the make lint-fix command

closes: #8112


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards. - n/a
  • Wrote unit and integration tests - n/a
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/) - n/a
  • Added relevant godoc comments. - n/a
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md - n/a
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes - n/a

Copy link
Contributor

@alexanderbez alexanderbez 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 doing this. I love this extension and I've been using it for a long time.

@robert-zaremba
Copy link
Collaborator

Had a chat with @ryanchristo . The default Markdown lint yields lot of errors, and many of them are are not auto fixable.
I suggest to create a config which won't yield many not auto fixable errors, and optionally later on we can update the config and fix other linter errors.

Makefile Outdated Show resolved Hide resolved
@ryanchristo
Copy link
Contributor Author

The default Markdown lint yields lot of errors, and many of them are are not auto fixable.

For clarification, we are using the same configuration as tendermint, not the default configuration.

I suggest to create a config which won't yield many not auto fixable errors, and optionally later on we can update the config and fix other linter errors.

It would be nice if we did not alter the configuration because it is currently the same configuration for tendermint but this would be one way to break up the work. I don't mind going through and fixing all the lint warnings if this needs to be done but it will be a lot of changes for one pull request so I just wanted to double check with others before moving forward.

@amaury1093
Copy link
Contributor

amaury1093 commented May 20, 2021

I'm fine with a 3k line diff in this PR, but maybe the commits could be well separated into 1/ auto-fix and 2/ manual fixes, to make the review easier (the reviewer can run auto-fix locally, and just review the manual fixes, and make sure the output diff is the same as this PR's)

(also want to be respectful of Ryan doing a lot of repetitive chore work, if there's too much work for one PR, we can consider muting some lints for now and doing multiple PRs).

@robert-zaremba
Copy link
Collaborator

If you want to manually fix few thousands lines of code then fair enough 💪 Just wanted to point that it's not very important.

@ryanchristo
Copy link
Contributor Author

I'm definitely reevaluating after further reviewing the warnings and changes. I'm also unsure about a couple configuration options such as the addition of angle brackets around links. I'm going to explore your initial suggestions @robert-zaremba. Thanks!

@github-actions github-actions bot added C:Cosmovisor Issues and PR related to Cosmovisor C:Rosetta Issues and PR related to Rosetta C:x/auth C:x/authz C:x/bank C:x/crisis C:x/distribution distribution module related C:x/evidence C:x/feegrant C:x/gov C:x/mint C:x/params C:x/slashing C:x/staking C:x/upgrade T: ADR An issue or PR relating to an architectural decision record T:Docs Changes and features related to documentation. labels May 20, 2021
@github-actions github-actions bot added the T: CI label May 20, 2021
@github-actions github-actions bot removed the T: CI label May 20, 2021
@ryanchristo
Copy link
Contributor Author

@marbar3778 I'm receiving the following error with the Atlas checks:

Run marbar3778/atlas_action@main

...

failed to publish module: {"error":"failed to query user token: record not found"}

Any ideas?

@ryanchristo ryanchristo marked this pull request as ready for review May 24, 2021 22:44
@ryanchristo ryanchristo requested a review from aaronc as a code owner May 24, 2021 22:44
Copy link
Contributor

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Collaborator

@robert-zaremba robert-zaremba left a comment

Choose a reason for hiding this comment

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

Good job.
If there is any follow up task (eg to change some rules), then let's create an issue for it.

@amaury1093
Copy link
Contributor

i was goingi to put automerge, but saw atlas failing. Do we want to fix that before merging? cc @alexanderbez @marbar3778

@ryanchristo
Copy link
Contributor Author

i was goingi to put automerge, but saw atlas failing. Do we want to fix that before merging? cc @alexanderbez @marbar3778

I messaged @marbar3778 in Discord and he said he was looking into this but it does not need to be solved before merging. It should be safe to add automerge.

@tac0turtle
Copy link
Member

yea feel free to merge. I need to dive into it.

@tac0turtle tac0turtle merged commit cb66c99 into master May 27, 2021
@tac0turtle tac0turtle deleted the ryan/8112-markdownlint branch May 27, 2021 15:31
larry0x pushed a commit to larry0x/cosmos-sdk that referenced this pull request May 22, 2023
* add markdownlint config

* update make lint commands

* update markdownlint config

* run make lint-fix

* fix empty link

* resuse docker container

* run lint-fix

* do not echo commands

Co-authored-by: ryanchrypto <12519942+ryanchrypto@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:Cosmovisor Issues and PR related to Cosmovisor C:Rosetta Issues and PR related to Rosetta C:x/auth C:x/authz C:x/bank C:x/crisis C:x/distribution distribution module related C:x/evidence C:x/feegrant C:x/gov C:x/mint C:x/params C:x/slashing C:x/staking C:x/upgrade T: ADR An issue or PR relating to an architectural decision record T:Docs Changes and features related to documentation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove trailing whitespace in markdown
5 participants