kaxil commented on code in PR #74549: URL: https://github.com/apache/airflow/pull/74549#discussion_r4247283840
########## devel-common/tests/unit/sphinx_exts/test_airflow_spelling.py: ########## @@ -0,0 +1,213 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +from __future__ import annotations + +import json +import sys +from pathlib import Path +from types import SimpleNamespace +from unittest import mock + +import enchant +import pytest +from docutils import nodes +from docutils.utils import new_document +from docutils.utils.smartquotes import smartchars, smartyPants +from sphinx.application import Sphinx +from sphinx.environment import BuildEnvironment +from sphinx.errors import SphinxError +from sphinxcontrib.spelling import filters + +SPHINX_EXTS_PATH = Path(__file__).parents[3] / "src" / "sphinx_exts" +if SPHINX_EXTS_PATH.as_posix() not in sys.path: + # The extensions are loaded by Sphinx from this directory and import each other by bare name. + sys.path.append(SPHINX_EXTS_PATH.as_posix()) + +from sphinx_exts import airflow_spelling # noqa: E402 + +KNOWN_WORDS = {"the", "word", "is", "spelled", "do", "Dag", "text", "quoted", "a"} + + +def _config(**overrides): + values = { + "spelling_ignore_wiki_words": True, + "spelling_ignore_acronyms": True, + "spelling_ignore_python_builtins": True, + "spelling_ignore_importable_modules": True, + "spelling_ignore_contributor_names": False, + "spelling_filters": [], + "spelling_word_list_filename": ["wordlist.txt"], + "spelling_lang": "en_US", + "tokenizer_lang": "en_US", + "spelling_exclude_patterns": [], + "airflow_spelling_output": "", + } + values.update(overrides) + return SimpleNamespace(**values) + + [email protected](autouse=True) +def dictionary_factory(monkeypatch): + pwl = mock.create_autospec(enchant.DictWithPWL, instance=True) + pwl.check.side_effect = KNOWN_WORDS.__contains__ + factory = mock.create_autospec(enchant.DictWithPWL, return_value=pwl) + monkeypatch.setattr(airflow_spelling.enchant, "DictWithPWL", factory) + return factory + + [email protected] +def dictionary(dictionary_factory): + return dictionary_factory.return_value + + [email protected] +def checker(tmp_path): + return airflow_spelling.SpellChecker(_config(), tmp_path) + + +class TestSpellChecker: + def test_reports_unknown_words_with_their_offset_in_the_text(self, checker): + text = "the zorp is\nspelled (badly) Dag's" + + assert list(checker.find_misspellings(text)) == [("zorp", 4), ("badly", 21)] + + def test_skips_what_the_spelling_builder_filters_skip(self, checker): + text = "API URLs WikiWord len [email protected] sys don't" + + assert list(checker.find_misspellings(text)) == [] + + def test_each_distinct_chunk_is_checked_once(self, checker, dictionary): + list(checker.find_misspellings("zorp")) + checks_for_first_sight = dictionary.check.call_count + + assert list(checker.find_misspellings("zorp zorp")) == [("zorp", 0), ("zorp", 5)] + assert dictionary.check.call_count == checks_for_first_sight + + @mock.patch.object(filters.ImportableModuleFilter, "_skip", autospec=True, return_value=False) + def test_importable_module_lookup_only_runs_for_words_still_misspelled(self, mock_skip, checker): + list(checker.find_misspellings("the zorp")) Review Comment: I don't think this can catch the regression it's named for. `the` is alphabetic and in the dictionary, so it takes the fast path and never reaches a tokenizer; if the two tokenizers were collapsed into one carrying every filter, `_skip` would still only see `zorp`. `"(the) zorp"` sends `the` through the tokenizer: it fails against a single all-filters tokenizer (`['the', 'zorp']`) and passes on this code. ########## dev/breeze/doc/03_developer_tasks.rst: ########## @@ -251,18 +251,8 @@ To build documentation in Breeze, use the ``build-docs`` command: Results of the build can be found in the ``generated/_build`` folder. -The documentation build consists of three steps: - -* verifying consistency of indexes -* building documentation -* spell checking - -You can choose only one stage of the two by providing ``--spellcheck-only`` or ``--docs-only`` after -extra ``--`` flag. - -.. code-block:: bash - - breeze build-docs --spellcheck-only +Spelling is checked while the documentation is built, so a single build reports both build and Review Comment: `contributing-docs/11_documentation_building.rst` still tells people to choose `--doc-only` or `--spelling-only` (around line 233) and shows `breeze build-docs --doc-only --clean fab` at line 240. Can that page be updated here as well? The enchant import error it quotes at line 92 will name `airflow_spelling` now, too. ########## .github/workflows/publish-docs-to-s3.yml: ########## @@ -375,7 +375,12 @@ jobs: > generated/_build/docs/sphinx-build-skipped.txt echo "sphinx-build-skipped=true" >> "${GITHUB_OUTPUT}" else - breeze build-docs ${SPHINX_INCLUDE_DOCS} --docs-only ${FAIL_ON_INVENTORIES} + # Refs whose breeze still has --docs-only check spelling in a second full build; skip it. + DOCS_ONLY="" + if grep -q '"--docs-only"' dev/breeze/src/airflow_breeze/commands/developer_commands.py; then + DOCS_ONLY="--docs-only" + fi + breeze build-docs ${SPHINX_INCLUDE_DOCS} ${DOCS_ONLY} ${FAIL_ON_INVENTORIES} Review Comment: For refs after this PR, publishing now fails on any spelling error, where `--docs-only` meant it never checked. Usually fine since CI checked the ref, but the "Apply patch commits" step cherry-picks onto the release ref and that combination was never spell-checked (a cherry-pick relying on a wordlist entry that only exists on main would do it). Is blocking the publish intended there, or should publish keep a way to skip the check? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
