-
Notifications
You must be signed in to change notification settings - Fork 79
Snow 2306184 ng config support 3 #2638
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.
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
base: main
Are you sure you want to change the base?
Conversation
|
|
||
| def __init__(self): | ||
| self._cache_file = _VersionCache._version_cache_file | ||
| self._cache_file = self._version_cache_file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The property _version_cache_file is now a method rather than a class variable, but it's still being evaluated immediately during initialization when assigned to self._cache_file. This defeats the purpose of lazy evaluation.
Consider modifying this approach to truly implement lazy loading:
def __init__(self):
self._cache_file = None # Initialize as None
@property
def _cache_file(self):
if self.__cache_file is None:
self.__cache_file = self._version_cache_file
return self.__cache_file
@_cache_file.setter
def _cache_file(self, value):
self.__cache_file = valueThis way, the property is only evaluated when first accessed, not during initialization.
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
54e7ad4 to
52151dd
Compare
| test_compiler.register(TestProcessor) | ||
| test_compiler.register(AcmeProcessor) | ||
|
|
||
| # TestProcessor is never invoked, otherwise calling its methods will make the test fail |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The class name was changed from TestProcessor to AcmeProcessor, but this comment still refers to TestProcessor. Please update the comment to refer to AcmeProcessor to maintain consistency with the renamed class.
| # TestProcessor is never invoked, otherwise calling its methods will make the test fail | |
| # AcmeProcessor is never invoked, otherwise calling its methods will make the test fail |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The class was renamed to avoid warnings during tests collection.
99a467d to
c32692c
Compare
68ccdbc to
ee5e0ce
Compare
ee5e0ce to
2484c26
Compare
sfc-gh-jwilkowski
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left 1 important question about connections_file_override which does not seem to be needed (maybe yet?). Otherwise a general comment would be about recurring issues across this PR:
- lots of local imports, where I don't think most of them would cause circular deps
- lots of comments that don't add much value
- I've seen this pattern repeating several times - maybe path resolver could go into SecurePath so that we can avoid repeating those?
# Resolve Windows short paths to prevent cleanup issues
resolved_tmp_dir = path_resolver(tmp_dir)
config_path = Path(resolved_tmp_dir) / "config.toml"
Otherwise, looks great!
| @property | ||
| def filename(self): | ||
| return self.path.path / _DEFAULT_LOG_FILENAME | ||
| from snowflake.cli.api.utils.path_utils import path_resolver |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: local import
|
|
||
| # Create ConfigSlice for connections.toml (same as singleton CONFIG_MANAGER) | ||
| connections_slice = ConfigSlice( | ||
| path=connections_file, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
path=self.connections_file_override or connections_file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually is that override even needed? We don't seem to be using it anywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With connector based singleton this was created by default. As we are dropping the connector version of singleton we need to have our own support for connections.toml
2484c26 to
66356e5
Compare
af729e4 to
304a27a
Compare
Pre-review checklist
Changes description
This PR refactors the Snowflake CLI configuration management by removing the module-level
CONFIG_MANAGERsingleton and replacing it with a factory-based approach managed through theCliGlobalContext. This change improves testability and eliminates test flakiness caused by shared global state.