refactor: introduce GlobalConfig.from_file method to init GlobalConfig #1655
Conversation
Code Coverage 🎉
|
|
|
||
| class GlobalConfig: | ||
| def __init__(self, global_config_path: str) -> None: | ||
| def __init__( |
There was a problem hiding this comment.
Do you think we could cover tests and use cases to capture this alteration, to ensure that this isn't changed in future?
There was a problem hiding this comment.
It's indirectly tested by from_file calling it, do you think about some other tests we can have?
There was a problem hiding this comment.
I can add those in a separate small PR.
| self, | ||
| content: Dict[str, Any], | ||
| is_yaml: bool, | ||
| original_path: str, |
There was a problem hiding this comment.
If we are passing the original_path, we could directly load the file into memory, making content redundant, as we are doing right now. How does this new __init__ benefit us?
There was a problem hiding this comment.
I see one advantage - easier unit testing, as a file does not need to be created.
As for the original_path, maybe we should remove it? It is used in dashboards to read the global config (we can access the content directly instead), plus handle_global_config_update - we could add a new parameter for path.
There was a problem hiding this comment.
+1 to what Kamił said
As for the original_path, maybe we should remove it? It is used in dashboards to read the global config (we can access the content directly instead), plus handle_global_config_update - we could add a new parameter for path.
Totally agree with you, I've been thinking about it but I'd like to do it in a separate small PR so it can be reviewed and merged quickly.
There was a problem hiding this comment.
Should we update the source code to simplify unit testing, or when we have to add optimizations and efficiencies to source code? This way, I think we would be proceeding with Classical unit testing instead of Mockist unit testing.
There was a problem hiding this comment.
In my opinion, if it's hard to write unit tests or it's hard to understand what's there - it's time to simplify. It might take couple of iterations to come up with the testable implementation for the feature which is completely fine by me.
| self, | ||
| content: Dict[str, Any], | ||
| is_yaml: bool, | ||
| original_path: str, |
There was a problem hiding this comment.
I see one advantage - easier unit testing, as a file does not need to be created.
As for the original_path, maybe we should remove it? It is used in dashboards to read the global config (we can access the content directly instead), plus handle_global_config_update - we could add a new parameter for path.
3 participants
Issue number: ADDON-78488
PR Type
What kind of change does this PR introduce?
Summary
Changes
This PR introduces a new method to initialize
GlobalConfiginstance -from_file. It accepts a path to the globalConfig file (can be JSON or YAML) and returns an instance ofGlobalConfig.It's a preparation work for #1625 PR.
User experience
N/A
Checklist
If an item doesn't apply to your changes, leave it unchecked.
Review
Tests
See the testing doc.
Demo/meeting:
Reviewers are encouraged to request meetings or demos if any part of the change is unclear