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

Feature : Added markdown-toc package to auto-generate toc with husky pre commit hook. #1280

Merged

Conversation

aialok
Copy link
Contributor

@aialok aialok commented Dec 25, 2023

What kind of change does this PR introduce?

  • Added update-toc script to pre-commit hook
  • This commit adds the "update-toc" script to the pre-commit hook in the .husky/pre-commit file. This script updates the table of contents in the INSTALLATION.md file using the markdown-toc package.

Issue Number:

Fixes #1234

Did you add tests for your changes?

  • No

Snapshots/Videos:

  • Here is clip of how table of content is getting generated on committing the changes
cinnamon-2023-12-25T140007+0530.webm

If relevant, did you update the documentation?

  • No

Summary

Add markdown-toc package for generating table of contents
This commit adds the "markdown-toc" package to the project's dependencies in the package.json file. This package is used to generate a table of contents in the INSTALLATION.md file.

Does this PR introduce a breaking change?

  • No

Other information

Have you read the contributing guide?

  • Yes

…o automatically update TOC in INSTALLATION.md using markdown-toc package.
Copy link

Our Pull Request Approval Process

We have these basic policies to make the approval process smoother for our volunteer team.

Testing Your Code

Please make sure your code passes all tests. Our test code coverage system will fail if these conditions occur:

  1. The overall code coverage drops below the target threshold of the repository
  2. Any file in the pull request has code coverage levels below the repository threshold
  3. Merge conflicts

The process helps maintain the overall reliability of the code base and is a prerequisite for getting your PR approved. Assigned reviewers regularly review the PR queue and tend to focus on PRs that are passing.

Reviewers

When your PR has been assigned reviewers contact them to get your code reviewed and approved via:

  1. comments in this PR or
  2. our slack channel

Reviewing Your Code

Your reviewer(s) will have the following roles:

  1. arbitrators of future discussions with other contributors about the validity of your changes
  2. point of contact for evaluating the validity of your work
  3. person who verifies matching issues by others that should be closed.
  4. person who gives general guidance in fixing your tests

CONTRIBUTING.md

Read our CONTRIBUTING.md file. Most importantly:

  1. PRs with issues not assigned to you will be closed by the reviewer
  2. Fix the first comment in the PR so that each issue listed automatically closes

Other

  1. 🎯 Please be considerate of our volunteers' time. Contacting the person who assigned the reviewers is not advised unless they ask for your input. Do not @ the person who did the assignment otherwise.
  2. Read the CONTRIBUTING.md file make

Copy link
Contributor

@palisadoes palisadoes left a comment

Choose a reason for hiding this comment

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

This should be done for all the markdown files in the riot directory. That would solve a universal problem. Please update.

@palisadoes
Copy link
Contributor

Please fix the failing typescript tests

@aialok
Copy link
Contributor Author

aialok commented Dec 25, 2023

Hey @palisadoes ! I have added a new update-toc.js file where it will execute markdown-toc -i "${file}" --bullets "-" for each .md file present in the talawa-admin.
Here is clip how it is working :

cinnamon-2023-12-25T224523+0530.webm

If it look good I will push the changes.

@palisadoes
Copy link
Contributor

  1. Yes, go ahead.
  2. Please merge your code with the latest upstream

Copy link

codecov bot commented Dec 25, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (eab9e4d) 96.57% compared to head (d0fbd33) 96.57%.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #1280   +/-   ##
========================================
  Coverage    96.57%   96.57%           
========================================
  Files          137      137           
  Lines         3417     3417           
  Branches       958      958           
========================================
  Hits          3300     3300           
  Misses         112      112           
  Partials         5        5           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@palisadoes palisadoes left a comment

Choose a reason for hiding this comment

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

  1. Why is this PR updating typescript and language JSON files?
  2. Please don't include them in the PR. Do not delete them from the repo.

@aialok
Copy link
Contributor Author

aialok commented Dec 25, 2023

  1. Why is this PR updating typescript and language JSON files?
  2. Please don't include them in the PR. Do not delete them from the repo.

Those changes are not mine. It is there because I have fetched the latest upstream which is committed by other.

@palisadoes
Copy link
Contributor

Though they may not be your edits, they must not be included in this PR. We only want to review what was required in the source issue

@aialok aialok force-pushed the installationdotmd-new-toc branch from 81db5f7 to 0aad977 Compare December 26, 2023 07:53
Copy link
Contributor

@palisadoes palisadoes left a comment

Choose a reason for hiding this comment

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

See comment

.husky/pre-commit Outdated Show resolved Hide resolved
@aialok aialok requested review from palisadoes December 27, 2023 19:26
Copy link
Contributor

@palisadoes palisadoes left a comment

Choose a reason for hiding this comment

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

See comment

package.json Outdated Show resolved Hide resolved
@palisadoes
Copy link
Contributor

  1. This solution doesn't use a script
  2. It'll also be added to the Git hooks

PTAL

@palisadoes
Copy link
Contributor

Is this integrated with the git hooks?

@aialok
Copy link
Contributor Author

aialok commented Dec 28, 2023

Is this integrated with the git hooks?

Yes, I have added command in husky pre commit hook.

Screenshot from 2023-12-28 13-32-02

@palisadoes
Copy link
Contributor

I thought about the universality of this solution. Will it work for developers running on the most OS platforms?

  1. Windows
  2. Mac
  3. Linux

For example, the find command is Unix specific

@aialok
Copy link
Contributor Author

aialok commented Dec 28, 2023

I thought about the universality of this solution. Will it work for developers running on the most OS platforms?

  1. Windows
  2. Mac
  3. Linux

For example, the find command is Unix specific

  • Hey, I try running in windows environment its not working as some of the UNIX command is not supported by Windows PowerShell and command prompt . Either we can add this to GitHub actions workflow and run this command. or we can roll back to node script which I wrote. I tried running node script in both windows and Linux and its working fine.

  • One more things which we can do is to write script for each platform.

  • Node Scripts

Screenshot 2023-12-28 133158

  • Bash Command

Screenshot 2023-12-28 134200

@palisadoes
Copy link
Contributor

  1. Create a typescript file and it to a scripts/githooks directory. It'll help with standardization
  2. The scripts directory already exists.
  3. Move any other git hooks scripts there too and update the hook for the new location

@aialok
Copy link
Contributor Author

aialok commented Dec 28, 2023

  1. Create a typescript file and it to a scripts/githooks directory. It'll help with standardization
  2. The scripts directory already exists.
  3. Move any other git hooks scripts there too and update the hook for the new location
  • Can I use javascript file as other script for testing in js and if we are using js then we dont need to compile ts to js and then run it with node.
  • It will be easy for us to run the script.

@palisadoes
Copy link
Contributor

  1. Create a typescript file and it to a scripts/githooks directory. It'll help with standardization
  2. The scripts directory already exists.
  3. Move any other git hooks scripts there too and update the hook for the new location
* Can I use javascript file as other script for `testing` in js and if we are using js then we dont need to compile ts to js and then run it with node.

* It will be easy for us to run the script.

OK

@aialok aialok force-pushed the installationdotmd-new-toc branch from abccb88 to aafa094 Compare December 28, 2023 18:52
@aialok
Copy link
Contributor Author

aialok commented Dec 29, 2023

@palisadoes , Please Have a look : )

  1. Create a typescript file and it to a scripts/githooks directory. It'll help with standardization
  2. The scripts directory already exists.
  3. Move any other git hooks scripts there too and update the hook for the new location
* Can I use javascript file as other script for `testing` in js and if we are using js then we dont need to compile ts to js and then run it with node.

* It will be easy for us to run the script.

OK

@palisadoes , Please have a look : )

@aialok
Copy link
Contributor Author

aialok commented Dec 29, 2023

Hi @palisadoes , could you please check this out? Thanks!

@palisadoes palisadoes merged commit d15cf33 into PalisadoesFoundation:develop Dec 29, 2023
8 of 9 checks passed
@aialok aialok deleted the installationdotmd-new-toc branch January 7, 2024 12:08
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