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 action for spellcheck and linkcheck #59

Merged
merged 5 commits into from
Jun 9, 2022

Conversation

scottyhq
Copy link
Contributor

@scottyhq scottyhq commented Jun 2, 2022

@scottyhq
Copy link
Contributor Author

scottyhq commented Jun 2, 2022

Can run spellcheck locally with (conda create -n codespell codespell) codespell --skip=./_build --ignore-words-list=hist,nd

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@scottyhq
Copy link
Contributor Author

scottyhq commented Jun 2, 2022

Lots of broken links, can fix those in a follow up PR https://xarray.pydata.org/en/latest/dask.html#map-blocks -> https://xarray.pydata.org/en/latest/user-guide/dask.html#map-blocks

some complicated ones are also reported as broken but do resolve https://docs.google.com/presentation/d/16CMY3g_OYr6fQplUZIDqVtG-SKZqsG8Ckwoj2oOqepU/edit#slide=id.g2b68f9254d_1_154

@scottyhq scottyhq requested a review from dcherian June 2, 2022 17:44
@max-sixty
Copy link

This is great!

Is there also a pre-commit hook for spelling? (I know for links, pre-commit.ci doesn't work because it doesn't have internet access)

@scottyhq
Copy link
Contributor Author

scottyhq commented Jun 7, 2022

Is there also a pre-commit hook for spelling?

Looks like there is! But I haven't used it. Should I add that as well or instead of the github action step? https://pre-commit.com/hooks.html

@max-sixty
Copy link

Should I add that as well or instead of the github action step?

Personally I would vote to aggregate them in pre-commit, because it's really easy to run locally. But your call!

(Also fine to merge this now to avoid conflicts and then resolve after)

@max-sixty
Copy link

We could also add the link check to pre-commit, add ci:\n skip: [markdown-link-check] to avoid it attempting in ci, but this is very refine-y, and the proposed code is already excellent.

@scottyhq
Copy link
Contributor Author

scottyhq commented Jun 9, 2022

I advocate for merging this as-is and adding to pre-commit later if desirable. While I agree it would be convenient, both of these checks can have lots of false positives and the linkchecking can hang with network failures. So they seem well suited to opt-in or PR-level checks. The jupyterbook linkchecker I think also deals with many more formats and markup (myst-markdown).

It does look possible to setup both opt-out (SKIP=codespell git commit -m "foo" ) or opt-in (pre-commit run --hook-stage manual codespell)

@scottyhq scottyhq requested a review from max-sixty June 9, 2022 17:49
@dcherian
Copy link
Contributor

dcherian commented Jun 9, 2022

I agree ^. Also moving quickly is beneficial at the moment given the upcoming SciPy deadline.

@dcherian dcherian merged commit 34c0bb2 into xarray-contrib:main Jun 9, 2022
@scottyhq scottyhq mentioned this pull request Jun 9, 2022
@max-sixty
Copy link

Very much agree re merging! Thanks for doing that!

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.

None yet

3 participants