[OneExplorer] Introduce BaseModelToCfgMap/CfgToCfgObjMap #1453
Conversation
dayo09
force-pushed
the
1102-refactor-pr
branch
2 times, most recently
from
23adff9 to
37d3e6b
Compare
This commit refactors OneStorage class to divide its functionalities into CfgToCfgObjMap and BaseModelToCfgMap. It includes unit tests. ONE-vscode-DCO-1.0-Signed-off-by: Dayoung Lee <[email protected]>
dayo09
force-pushed
the
1102-refactor-pr
branch
from
37d3e6b to
d92f8e4
Compare
| * Update the data when cfg file's path is changed or the content is changed. | ||
| * @param type NodeType | ||
| * @param path config path to update | ||
| * @param newpath (optional) if exists, change the file path |
There was a problem hiding this comment.
I cannot fully understand this method to review. 😓 When will this method be called without the newpath? AFAIU it will reload the ConfigObj with the exactly same path before.
There was a problem hiding this comment.
When will this method be called without the newpath?
- When a config file is changed and
onChangedFileevent is triggered -> update the CfgObj (newpath === undefined) with the same old key - When a config file is renamed and
onDidRenameFileevent is triggered -> update the CfgObj (newpath !== undefined) with the new key
*The onDidRenameFiles event is not yet added to ONE Explorer, it's planned after this PR. You can see the rough idea here https://github.com/Samsung/ONE-vscode/pull/1443/files#diff-6a7e327eaf6ee1aa5232d0b8c719e823efaae072d7793421095dc0f9eb8229d5R450-R470)
| BaseModelToCfgMap as _unit_test_BaseModelToCfgMap, | ||
| CfgToCfgObjMap as _unit_test_CfgToCfgObjMap, |
There was a problem hiding this comment.
This is not test-related files but they are exported as _unit_test_*, why...?
There was a problem hiding this comment.
This PR includes unit test files.
Those maps are designed to be only used within OneStorage, internally. They are not to be exported. However, to test them more deeply, it's required that the unit test file imports them. So I put _unit_test prefix to them.
There was a problem hiding this comment.
I see it :)
Just FYI, there are some workaround patterns such as https://stackoverflow.com/a/54116079 and https://stackoverflow.com/a/65422568.
There was a problem hiding this comment.
Yes, actually the workaround used in the links are the same to this - it's by aliasing its internal functions when exporting :-D
| @@ -14,6 +14,7 @@ | |||
| * limitations under the License. | |||
| */ | |||
|
|
|||
| import {assert} from 'chai'; | |||
There was a problem hiding this comment.
AKAIK, chai is for writing testcases.
Maybe following is what you intended?
| import {assert} from 'chai'; | |
| import {assert} from 'assert'; |
There was a problem hiding this comment.
AKAIK, chai is for writing testcases.
I thought chai is more advanced assertion library, supporting BDD also.
https://www.chaijs.com/guide/styles/
I searched stackoverflow. It seems there is not exact guide about not using chai in place of assert. It seems there is no reason not to do so.
This commit refactors OneStorage class
to divide its functionalities into CfgToCfgObjMap and BaseModelToCfgMap.
It includes unit tests.
ONE-vscode-DCO-1.0-Signed-off-by: Dayoung Lee [email protected]
Related to #1442
From #1446
Waiting for #1452