kevinjqliu commented on code in PR #1464: URL: https://github.com/apache/iceberg-python/pull/1464#discussion_r1901245337
########## mkdocs/docs/api.md: ########## @@ -49,7 +49,7 @@ catalog: and loaded in python by calling `load_catalog(name="hive")` and `load_catalog(name="rest")`. -This information must be placed inside a file called `.pyiceberg.yaml` located either in the `$HOME` or `%USERPROFILE%` directory (depending on whether the operating system is Unix-based or Windows-based, respectively) or in the `$PYICEBERG_HOME` directory (if the corresponding environment variable is set). +This information must be placed inside a file called `.pyiceberg.yaml` located either in the `$HOME` or `%USERPROFILE%` directory (depending on whether the operating system is Unix-based or Windows-based, respectively), in the current working directory, or in the `$PYICEBERG_HOME` directory (if the corresponding environment variable is set). Review Comment: nit: include warning about accidentally checking in secrets with git when using the current working directory ########## tests/utils/test_config.py: ########## @@ -93,3 +94,76 @@ def test_from_configuration_files_get_typed_value(tmp_path_factory: pytest.TempP assert Config().get_bool("legacy-current-snapshot-id") assert Config().get_int("max-workers") == 4 + + +@pytest.mark.parametrize( + "config_setup, expected_result", + [ + # Validate lookup works with: config > home > cwd + ( Review Comment: nit: for test readability, use `["PYICEBERG_HOME", "HOME", and "CURRENT"]` and replace `both` with a list ["HOME", "CURRENT"] ########## tests/utils/test_config.py: ########## @@ -93,3 +94,76 @@ def test_from_configuration_files_get_typed_value(tmp_path_factory: pytest.TempP assert Config().get_bool("legacy-current-snapshot-id") assert Config().get_int("max-workers") == 4 + + +@pytest.mark.parametrize( + "config_setup, expected_result", + [ + # Validate lookup works with: config > home > cwd + ( Review Comment: looks like the parameterize test is testing 1. PYICEBERG_HOME 2. HOME 3. CURRENT 4. None 5. "both" / ["HOME", "CURRENT"] i'd add a test for all 3 ########## mkdocs/docs/configuration.md: ########## @@ -28,7 +28,7 @@ hide: There are three ways to pass in configuration: -- Using the `~/.pyiceberg.yaml` configuration file +- Using the `.pyiceberg.yaml` configuration file stored in either the directory specified by the `PYICEBERG_HOME` environment variable, the home directory, or current working directory. Review Comment: nit: move the extra info about where the file is located down to L37. also i think its valuable to include a warning about accidentally checking in secrets with git when using the current working directory -- 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: issues-unsubscr...@iceberg.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org For additional commands, e-mail: issues-h...@iceberg.apache.org