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
+
+
[email protected](
+ "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
+
+
[email protected](
+ "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: [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]
