Copilot commented on code in PR #4584: URL: https://github.com/apache/cassandra/pull/4584#discussion_r2730153320
########## pylib/cqlshlib/test/test_docspath.py: ########## @@ -0,0 +1,140 @@ +# 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. + +import os +import tempfile +from unittest.mock import patch + +from .basecase import BaseTestCase +from cqlshlib.cqlshmain import get_docspath, _get_docs_from_package_resource, Shell + + +class TestGetDocspath(BaseTestCase): + """ + Tests for the get_docspath() function. + + Verifies that CQL documentation paths are resolved according to the + function's priority logic. + """ + + def test_local_dev_path(self): + """Local doc/cql3/CQL.html takes precedence over all other paths.""" + with tempfile.TemporaryDirectory() as tmpdir: + docs_dir = os.path.join(tmpdir, 'doc', 'cql3') + os.makedirs(docs_dir) + docs_file = os.path.join(docs_dir, 'CQL.html') + with open(docs_file, 'w') as f: + f.write('<html></html>') + + result = get_docspath(tmpdir) + + self.assertTrue(result.startswith('file://')) + self.assertIn('doc/cql3/CQL.html', result) + self.assertEqual(result, 'file://' + os.path.abspath(docs_file)) + + def test_linux_package_path(self): + """Linux package path when local path doesn't exist.""" + with tempfile.TemporaryDirectory() as tmpdir: + with patch('os.path.exists') as mock_exists: + def exists_side_effect(path): + if path == os.path.join(tmpdir, 'doc', 'cql3', 'CQL.html'): + return False + if path == '/usr/share/doc/cassandra/CQL.html': + return True + return False + + mock_exists.side_effect = exists_side_effect + + result = get_docspath(tmpdir) + + self.assertEqual(result, 'file:///usr/share/doc/cassandra/CQL.html') + + def test_macos_path(self): + """macOS path when local and Linux paths don't exist.""" + with tempfile.TemporaryDirectory() as tmpdir: + with patch('os.path.exists') as mock_exists: + def exists_side_effect(path): + if path == os.path.join(tmpdir, 'doc', 'cql3', 'CQL.html'): + return False + if path == '/usr/share/doc/cassandra/CQL.html': + return False + if path == '/usr/local/share/doc/cassandra/CQL.html': + return True + return False + + mock_exists.side_effect = exists_side_effect + + result = get_docspath(tmpdir) + + self.assertEqual(result, 'file:///usr/local/share/doc/cassandra/CQL.html') + + def test_package_resource(self): + """Package resource when filesystem paths don't exist.""" + with tempfile.TemporaryDirectory() as tmpdir: + with patch('os.path.exists', return_value=False): + with patch('cqlshlib.cqlshmain._get_docs_from_package_resource') as mock_resource: + mock_resource.return_value = 'file:///some/resource/path/CQL.html' + + result = get_docspath(tmpdir) + + self.assertEqual(result, 'file:///some/resource/path/CQL.html') + mock_resource.assert_called_once() + + def test_online_url_fallback(self): + """Online documentation URL when all local paths fail.""" + with tempfile.TemporaryDirectory() as tmpdir: + with patch('os.path.exists', return_value=False): + with patch('cqlshlib.cqlshmain._get_docs_from_package_resource', return_value=None): + result = get_docspath(tmpdir) + + self.assertEqual(result, Shell.DEFAULT_CQLDOCS_URL) + + +class TestGetDocsFromPackageResource(BaseTestCase): + """Tests for the _get_docs_from_package_resource() function.""" + + def test_returns_none_on_import_error(self): + """Should return None if importlib.resources is not available.""" + with patch.dict('sys.modules', {'importlib.resources': None}): + with patch('cqlshlib.cqlshmain.sys.version_info', (3, 9)): + with patch('builtins.__import__', side_effect=ImportError): + result = _get_docs_from_package_resource() + self.assertIsNone(result) + + def test_returns_none_when_resource_not_found(self): + """Should return None if the resource file doesn't exist.""" + from unittest.mock import MagicMock + from contextlib import contextmanager + + @contextmanager + def mock_as_file(resource): + mock_path = MagicMock() + mock_path.exists.return_value = False + yield mock_path + + with patch('cqlshlib.cqlshmain.sys.version_info', (3, 9)): + with patch('importlib.resources.files') as mock_files: + with patch('importlib.resources.as_file', mock_as_file): + mock_files.return_value.joinpath.return_value = MagicMock() + result = _get_docs_from_package_resource() + self.assertIsNone(result) + + def test_exception_handling(self): + """Should handle exceptions gracefully and return None.""" + with patch('cqlshlib.cqlshmain.sys.version_info', (3, 9)): + with patch('importlib.resources.files', side_effect=Exception("Test error")): + result = _get_docs_from_package_resource() + self.assertIsNone(result) Review Comment: All tests in TestGetDocsFromPackageResource mock sys.version_info to (3, 9), which means the Python 3.8 compatibility code path (lines 2170-2175 in cqlshmain.py) is never tested. Add test cases that set sys.version_info to (3, 8) to ensure the Python 3.8 compatibility path works correctly. ########## pylib/setup.py: ########## @@ -30,6 +30,10 @@ def get_extensions(): setup( name="cassandra-pylib", description="Cassandra Python Libraries", - packages=["cqlshlib"], + packages=["cqlshlib", "cqlshlib.resources"], + package_data={ + "cqlshlib.resources": ["CQL.html", "CQL.css"], + }, + include_package_data=True, Review Comment: The 'include_package_data=True' setting is redundant when explicitly using 'package_data'. The 'include_package_data' option is used to include files specified in MANIFEST.in, but since you're explicitly listing the files in 'package_data', this setting is unnecessary. Consider removing it to avoid potential confusion or conflicts. If you want to keep it for other files, ensure you have a MANIFEST.in file or clarify the intent. ```suggestion ``` ########## pylib/cqlshlib/cqlshmain.py: ########## @@ -2125,14 +2125,57 @@ def read_options(cmdlineargs, parser, config_file, cql_dir, environment=os.envir def get_docspath(path): - cqldocs_url = Shell.DEFAULT_CQLDOCS_URL - if os.path.exists(path + '/doc/cql3/CQL.html'): - # default location of local CQL.html - cqldocs_url = 'file://' + path + '/doc/cql3/CQL.html' - elif os.path.exists('/usr/share/doc/cassandra/CQL.html'): - # fallback to package file - cqldocs_url = 'file:///usr/share/doc/cassandra/CQL.html' - return cqldocs_url + """ + Determine the URL for CQL documentation. + + Priority order: + 1. Local development/tarball path: {path}/doc/cql3/CQL.html + 2. Linux package path: /usr/share/doc/cassandra/CQL.html + 3. macOS path: /usr/local/share/doc/cassandra/CQL.html + 4. Bundled package resource (for pip installs, etc.) + 5. Online documentation URL (fallback) + """ + # Check local dev/tarball path + local_path = os.path.join(path, 'doc', 'cql3', 'CQL.html') + if os.path.exists(local_path): + return 'file://' + os.path.abspath(local_path) + + # Check system package installation paths + for system_path in ['/usr/share/doc/cassandra/CQL.html', + '/usr/local/share/doc/cassandra/CQL.html']: + if os.path.exists(system_path): + return 'file://' + system_path + + # Try to load from package resources + resource_url = _get_docs_from_package_resource() + if resource_url: + return resource_url + + # Fall back to online documentation + return Shell.DEFAULT_CQLDOCS_URL + + +def _get_docs_from_package_resource(): + """ + Attempt to load CQL documentation from package resources. + Returns a file:// URL to the resource, or None if unavailable. + """ + try: + if sys.version_info >= (3, 9): + from importlib.resources import files, as_file + resource = files('cqlshlib.resources').joinpath('CQL.html') + with as_file(resource) as path: + if path.exists(): + return 'file://' + str(path.resolve()) + else: + # Python 3.8 compatibility + from importlib.resources import path as resource_path + with resource_path('cqlshlib.resources', 'CQL.html') as path: + if path.exists(): + return 'file://' + str(path.resolve()) + except Exception: Review Comment: 'except' clause does nothing but pass and there is no explanatory comment. ```suggestion except Exception: # Any error while loading the bundled CQL docs is non-fatal; fall back to other locations. ``` ########## pylib/cqlshlib/cqlshmain.py: ########## @@ -2125,14 +2125,57 @@ def read_options(cmdlineargs, parser, config_file, cql_dir, environment=os.envir def get_docspath(path): - cqldocs_url = Shell.DEFAULT_CQLDOCS_URL - if os.path.exists(path + '/doc/cql3/CQL.html'): - # default location of local CQL.html - cqldocs_url = 'file://' + path + '/doc/cql3/CQL.html' - elif os.path.exists('/usr/share/doc/cassandra/CQL.html'): - # fallback to package file - cqldocs_url = 'file:///usr/share/doc/cassandra/CQL.html' - return cqldocs_url + """ + Determine the URL for CQL documentation. + + Priority order: + 1. Local development/tarball path: {path}/doc/cql3/CQL.html + 2. Linux package path: /usr/share/doc/cassandra/CQL.html + 3. macOS path: /usr/local/share/doc/cassandra/CQL.html + 4. Bundled package resource (for pip installs, etc.) + 5. Online documentation URL (fallback) + """ + # Check local dev/tarball path + local_path = os.path.join(path, 'doc', 'cql3', 'CQL.html') + if os.path.exists(local_path): + return 'file://' + os.path.abspath(local_path) + + # Check system package installation paths + for system_path in ['/usr/share/doc/cassandra/CQL.html', + '/usr/local/share/doc/cassandra/CQL.html']: + if os.path.exists(system_path): + return 'file://' + system_path + + # Try to load from package resources + resource_url = _get_docs_from_package_resource() + if resource_url: + return resource_url + + # Fall back to online documentation + return Shell.DEFAULT_CQLDOCS_URL + + +def _get_docs_from_package_resource(): + """ + Attempt to load CQL documentation from package resources. + Returns a file:// URL to the resource, or None if unavailable. + """ + try: + if sys.version_info >= (3, 9): + from importlib.resources import files, as_file + resource = files('cqlshlib.resources').joinpath('CQL.html') + with as_file(resource) as path: + if path.exists(): + return 'file://' + str(path.resolve()) + else: + # Python 3.8 compatibility + from importlib.resources import path as resource_path + with resource_path('cqlshlib.resources', 'CQL.html') as path: + if path.exists(): + return 'file://' + str(path.resolve()) Review Comment: The context manager pattern here is problematic. The 'as_file' context manager may create temporary files that are cleaned up when exiting the context (which happens before returning). Since the returned URL will be used later when users run HELP commands, the file may no longer exist. The proper approach is to check if the resource exists before using as_file, and ensure the resource is accessible. For Python 3.9+, you can use 'files()' to get a Traversable and check 'is_file()' on it. If it exists in a package that's already on the filesystem, as_file will return the actual path (no cleanup issue). However, if the package is in a zip file, the extracted temp file will be cleaned up immediately after return. Consider one of these alternatives: 1. Check resource.is_file() before using as_file, and handle the zip case differently 2. Extract and cache the resource to a permanent location at startup 3. Keep the context manager open for the shell's lifetime by managing it at a higher scope ########## build.xml: ########## @@ -506,6 +509,16 @@ </wikitext-to-html> </target> + <target name="copy-cql-docs-to-pylib" depends="generate-cql-html" + description="Copy CQL documentation to pylib resources for packaging"> + <copy todir="${pylib.resources}" failonerror="true"> + <fileset dir="doc/cql3"> + <include name="CQL.html"/> + <include name="CQL.css"/> + </fileset> + </copy> + </target> Review Comment: The copy task should ensure the target directory exists before attempting to copy files. While the directory should exist due to the __init__.py file in version control, it's a best practice to add mkdir to handle edge cases or if the directory structure is ever modified. Consider adding a mkdir task before the copy operation. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
