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

Add linting steps to doc-automation workflow #1855

Merged

Conversation

sadra-barikbin
Copy link
Contributor

spellcheck and linkcheck was added to workflow yml file.

Checklist:

  • Did you have fun?

@sadra-barikbin sadra-barikbin mentioned this pull request Sep 10, 2022
1 task
@codecov
Copy link

codecov bot commented Sep 10, 2022

Codecov Report

Merging #1855 (a525833) into master (b2f80d2) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1855   +/-   ##
=======================================
  Coverage   44.95%   44.95%           
=======================================
  Files          63       63           
  Lines        2609     2609           
  Branches       56       56           
=======================================
  Hits         1173     1173           
  Misses       1436     1436           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

.github/workflows/doc-automation.yml Outdated Show resolved Hide resolved
@msaroufim
Copy link
Member

Thanks @sadra-barikbin this is a good step forward, to merge this here's what's left

  1. The link checker script is working so can you fix all those links? Otherwise all future PRs will will temporarily fail this check until someone does do this. Also check that the output is correct for example I saw some errors pointed out in the ipex example that seemed just fine to me
  2. For the spellchecking, it should only run on the md files that were changed in the PR which you can get like so
    - name: Get specific changed files
    and when the check does fail you should tell people what they need to do either fix the typos or add a new word in the dictionary

@msaroufim
Copy link
Member

msaroufim commented Sep 12, 2022

I noticed you're fixing some links, an important detail is for links in docs folder you need absolute links like a direct link GitHub.com/etc.. if you're linking to anything outside of docs otherwise the generated link will be broken. Anything that includes a .. is likely to be broken for example

I realize this is a bit messy but thank you for patience, this work will save us a lot of time.

@sadra-barikbin
Copy link
Contributor Author

Regarding those links, link-checker didn't raise an error. Where exactly you mean those links would break?

@msaroufim
Copy link
Member

Here's a good example https://github.com/pytorch/serve/pull/1794/files

@sadra-barikbin sadra-barikbin force-pushed the Feature-add-some-steps-to-lint-process branch from 1992d57 to a799657 Compare September 14, 2022 22:45
@sadra-barikbin
Copy link
Contributor Author

sadra-barikbin commented Sep 14, 2022

I did. Some link errors need your review.

@sadra-barikbin
Copy link
Contributor Author

Is spellcheck really a necessary thing? New words, sequences etc. must get added to its dictionary again and again. We could fix all typos now and remove this step. From now on, typos would most likely be fixed during PR reviews.

@sadra-barikbin sadra-barikbin force-pushed the Feature-add-some-steps-to-lint-process branch from 0f8ad73 to 82971e4 Compare September 14, 2022 23:03
@msaroufim
Copy link
Member

Spellchecking is definitely lower priority than finding broken links we can finish that first

@sadra-barikbin
Copy link
Contributor Author

@msaroufim any blocker?

@msaroufim
Copy link
Member

msaroufim commented Sep 18, 2022

The Lint jobs are both red, they should be green for us to merge this specific change. Spellchecking needs to be progressive (only apply to changed files) as we discussed or not used

Also should resolve the merge conflict. Let me know if you have any questions.

@sadra-barikbin sadra-barikbin force-pushed the Feature-add-some-steps-to-lint-process branch from ecdd985 to 59d0d29 Compare September 18, 2022 14:50
@sadra-barikbin
Copy link
Contributor Author

sadra-barikbin commented Sep 19, 2022

@msaroufim , Some links need your review:

  • I could not find a file named Transformer_handler_generalized_v2.py in the repo. referenced in kubernetes/kserve/kf_request_json/v2/bert/README.md. By the way there is examples/Huggingface_Transformers/Transformer_handler_generalized.py
  • [input](10000)s cause error in benchmarks/sample_report.md and I don't know their purpose.
  • I can't find https://github.com/pytorch/serve/tree/master/test/benchmark, referenced in CONTRIBUTING.md
  • I can't find https://github.com/pytorch/serve/settings/actions/runners referenced in benchmarks/README.md

@sadra-barikbin
Copy link
Contributor Author

The last commit is about a link which due to a bug in link-check (which markdown-link-check makes use of), action raises an error. I filed an issue for them but for now I did a workaround for that link.

@msaroufim
Copy link
Member

msaroufim commented Sep 21, 2022

Great to see the lint spellcheck now green

For the links here are some answers to your specific questions

@sadra-barikbin sadra-barikbin force-pushed the Feature-add-some-steps-to-lint-process branch from f52cefc to f11a32e Compare September 21, 2022 21:57
Copy link
Member

@msaroufim msaroufim left a comment

Choose a reason for hiding this comment

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

Heroic effort thank you!

@sadra-barikbin sadra-barikbin force-pushed the Feature-add-some-steps-to-lint-process branch from 3a04eb2 to 3ac7a92 Compare September 26, 2022 06:33
@sadra-barikbin
Copy link
Contributor Author

@msaroufim , This repo isn't actively maintained?

@msaroufim
Copy link
Member

It is, folks are just busy with other stuff @lxning @namannandan @agunapal @rohithkrn can one of you please take a look?

@msaroufim msaroufim requested review from namannandan and removed request for maaquib and rohithkrn October 11, 2022 00:58
.github/workflows/lint.yml Outdated Show resolved Hide resolved
.github/workflows/lint.yml Show resolved Hide resolved
ts_scripts/spellcheck_conf/wordlist.txt Outdated Show resolved Hide resolved
@msaroufim
Copy link
Member

@sadra-barikbin please address the last piece of feedback from @namannandan, get the spellcheck job to be green and we should be good to merge this

@sadra-barikbin sadra-barikbin requested review from msaroufim and namannandan and removed request for agunapal and msaroufim October 13, 2022 20:51
@sadra-barikbin
Copy link
Contributor Author

Done

@msaroufim msaroufim merged commit 3a7187c into pytorch:master Oct 13, 2022
@sadra-barikbin sadra-barikbin deleted the Feature-add-some-steps-to-lint-process branch October 14, 2022 14:21
jagadeeshi2i pushed a commit to jagadeeshi2i/serve that referenced this pull request Nov 1, 2022
* Add linting steps to doc-automation workflow

* Move changes to lint.yml

* Some improvements

* Fix bug

* Some enhancements

* Fix bug

* Fix some links, a few typos and also linting steps

* Fix intel-extension links and some others

* Fix some typos

* Fix remaining typos

* Fix some other links

* Fix bug

* Add some words to wordllist.txt

* Add some other words to wordlist.txt

* Fix some links and improve markdown-lin-check config

* Fix bug

* Temporary fix a link

* Fix 4 remaining links

* Fix typos

* Fix typos

* Fix bug

* Improve spellcheck and fix misspellings

* Clean wordlist.txt abit

* Fix GitHub 403 errors

From tcort/markdown-link-check#201 (comment)

* Fix md-link-check version

* Fix a link

Co-authored-by: Mark Saroufim <marksaroufim@fb.com>
jagadeeshi2i pushed a commit to jagadeeshi2i/serve that referenced this pull request Nov 3, 2022
* Add linting steps to doc-automation workflow

* Move changes to lint.yml

* Some improvements

* Fix bug

* Some enhancements

* Fix bug

* Fix some links, a few typos and also linting steps

* Fix intel-extension links and some others

* Fix some typos

* Fix remaining typos

* Fix some other links

* Fix bug

* Add some words to wordllist.txt

* Add some other words to wordlist.txt

* Fix some links and improve markdown-lin-check config

* Fix bug

* Temporary fix a link

* Fix 4 remaining links

* Fix typos

* Fix typos

* Fix bug

* Improve spellcheck and fix misspellings

* Clean wordlist.txt abit

* Fix GitHub 403 errors

From tcort/markdown-link-check#201 (comment)

* Fix md-link-check version

* Fix a link

Co-authored-by: Mark Saroufim <marksaroufim@fb.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code-quality p0 high priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants