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

Reply via email to