Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactor default config location resolution to use pathlib instead of os #2017

Merged
merged 5 commits into from
Aug 8, 2024
Merged

Refactor default config location resolution to use pathlib instead of os #2017

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1eb0850
use pathlib instead of os to determine config location
sfc-gh-pczajka Aug 7, 2024
e97721f
update description.md
sfc-gh-pczajka Aug 7, 2024
1640f35
lint
sfc-gh-pczajka Aug 7, 2024
21a2dfe
Update DESCRIPTION.md
sfc-gh-pczajka Aug 8, 2024
7ad0da4
Merge branch 'main' into SNOW-1616595-fix-default-config-location-on-…
sfc-gh-pczajka Aug 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions DESCRIPTION.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ Source code is also available at: https://github.com/snowflakedb/snowflake-conne

- v3.12.1(TBD)
- Fixed a bug that session token is logged when renewing session.
- Use `pathlib` instead of `os` for default config file location resolution.
- Fixed a bug that disabling client telemetry does not work.

- v3.12.0(July 24,2024)
Expand Down
8 changes: 4 additions & 4 deletions src/snowflake/connector/sf_dirs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,12 @@ def _resolve_platform_dirs() -> PlatformDirsProto:
"appname": "snowflake",
"appauthor": False,
}
snowflake_home = os.path.expanduser(
snowflake_home = pathlib.Path(
os.environ.get("SNOWFLAKE_HOME", "~/.snowflake/"),
)
if os.path.exists(snowflake_home):
Comment on lines -34 to -37
Copy link
Collaborator

@sfc-gh-aling sfc-gh-aling Aug 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm just curious is there a bug related to the previous implementation?

I checked the code on windows and think they are effectively equivalent, backslash will be handled by the path.Pathlib called in SFPlatformDirs constructor.

or it's more about code improvement?

import os
import pathlib

assert pathlib.Path(os.path.expanduser("~/.snowflake/")) == pathlib.Path(str(pathlib.Path("~/.snowflake/").expanduser()))

Copy link
Contributor Author

@sfc-gh-pczajka sfc-gh-pczajka Aug 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahhh, I see - I didn't notice that this is handled via pathlib later on. I think this might be worth adding anyway, as the code becomes less confusing (I honestly thought that there was a bug in there), although the connector works properly.
I've updated the PR title and description.md

).expanduser()
if snowflake_home.exists():
return SFPlatformDirs(
snowflake_home,
str(snowflake_home),
**platformdir_kwargs,
)
else:
Expand Down
8 changes: 5 additions & 3 deletions test/unit/test_configmanager.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -522,9 +522,11 @@ def test_sf_dirs(tmp_path, version):


def test_config_file_resolution_sfdirs_default():
default_loc = os.path.expanduser("~/.snowflake")
existed_before = os.path.exists(default_loc)
os.makedirs(default_loc, exist_ok=True)
from pathlib import Path

default_loc = Path("~/.snowflake").expanduser()
existed_before = default_loc.exists()
default_loc.mkdir(exist_ok=True)
try:
assert isinstance(_resolve_platform_dirs(), SFPlatformDirs)
finally:
Expand Down
Loading