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.

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

[OneExplorer] Introduce BaseModelToCfgMap/CfgToCfgObjMap #1453

Merged
merged 2 commits into from
Nov 9, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
230 changes: 167 additions & 63 deletions src/OneExplorer/OneStorage.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
* limitations under the License.
*/

import {assert} from 'chai';
Copy link
Contributor

Choose a reason for hiding this comment

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

AKAIK, chai is for writing testcases.
Maybe following is what you intended?

Suggested change
import {assert} from 'chai';
import {assert} from 'assert';

Copy link
Contributor Author

@dayo09 dayo09 Nov 9, 2022

Choose a reason for hiding this comment

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

@llFreetimell

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.

import * as fs from 'fs';
import * as path from 'path';
import * as vscode from 'vscode';
Expand All @@ -24,12 +25,157 @@ import {Logger} from '../Utils/Logger';
import {ConfigObj} from './ConfigObject';
import {Node, NodeType} from './OneExplorer';

interface StringListMap {
[key: string]: string[];
export {
BaseModelToCfgMap as _unit_test_BaseModelToCfgMap,
CfgToCfgObjMap as _unit_test_CfgToCfgObjMap,
Comment on lines +29 to +30
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not test-related files but they are exported as _unit_test_*, why...?

Copy link
Contributor Author

@dayo09 dayo09 Nov 9, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see it :)
Just FYI, there are some workaround patterns such as https://stackoverflow.com/a/54116079 and https://stackoverflow.com/a/65422568.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, actually the workaround used in the links are the same to this - it's by aliasing its internal functions when exporting :-D

};

class CfgToCfgObjMap {
private _map: Map<string, ConfigObj>;

constructor() {
this._map = new Map<string, ConfigObj>();
}

public init(cfgList: string[]) {
cfgList.forEach(cfg => {
const cfgObj = ConfigObj.createConfigObj(vscode.Uri.file(cfg));
if (cfgObj) {
this._map.set(cfg, cfgObj);
}
});
}

get size() {
return this._map.size;
}

public get(path: string) {
return this._map.get(path);
}

public reset(type: NodeType, path: string) {
switch (type) {
case NodeType.config:
this._map.delete(path);
break;
case NodeType.baseModel:
case NodeType.product:
case NodeType.directory:
default:
assert.isOk(false, `Cannot reach here`);
break;
}
}

/**
* 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
Comment on lines +72 to +75
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@dayo09 dayo09 Nov 7, 2022

Choose a reason for hiding this comment

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

When will this method be called without the newpath?

  • When a config file is changed and onChangedFile event is triggered -> update the CfgObj (newpath === undefined) with the same old key
  • When a config file is renamed and onDidRenameFile event 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)

*/
public update(type: NodeType, path: string, newpath?: string) {
switch (type) {
case NodeType.config: {
this._map.delete(path);

if (newpath) {
path = newpath;
}
const cfgObj = ConfigObj.createConfigObj(vscode.Uri.file(path));
if (cfgObj) {
this._map.set(path, cfgObj);
}

break;
}
case NodeType.baseModel:
case NodeType.product:
case NodeType.directory:
default:
assert.isOk(false, `Cannot reach here`);
break;
}
}
}

interface ConfigObjMap {
[key: string]: ConfigObj;
class BaseModelToCfgMap {
private _map: Map<string, string[]>;

constructor() {
this._map = new Map<string, string[]>();
}

public init(cfgList: string[], cfgToCfgObjMap: CfgToCfgObjMap) {
cfgList.forEach(cfg => {
const cfgObj = cfgToCfgObjMap.get(cfg);

(cfgObj ? cfgObj.getBaseModelsExists : []).forEach(artifact => {
this._map.get(artifact.path) ?
this._map.set(artifact.path, this._map.get(artifact.path)!.concat([cfg])) :
this._map.set(artifact.path, [cfg]);
});
});
}

get size() {
return this._map.size;
}

public get(path: string) {
return this._map.get(path);
}

public reset(type: NodeType, path: string) {
switch (type) {
case NodeType.baseModel:
this._map.delete(path);
break;
case NodeType.config:
this._map.forEach((cfgs, key, map) => {
if (cfgs.includes(path)) {
cfgs = cfgs.filter(cfg => cfg !== path);
map.set(key, cfgs);
}
});
break;
case NodeType.product:
case NodeType.directory:
default:
assert.isOk(false, `Cannot reach here`);
break;
}
}

public update(type: NodeType, oldpath: string, newpath: string) {
switch (type) {
case NodeType.baseModel: {
const value = this._map.get(oldpath);

if (!value) {
return;
}

this._map.delete(oldpath);
this._map.set(newpath, value);

break;
}
case NodeType.config:
this._map.forEach((cfgs, key, map) => {
if (cfgs.includes(oldpath)) {
cfgs = cfgs.map(cfg => (cfg === oldpath) ? newpath : cfg);
map.set(key, cfgs);
}
});
break;
case NodeType.product:
case NodeType.directory:
default:
assert.isOk(false, `Cannot reach here`);
break;
}
}
}

/**
Expand Down Expand Up @@ -62,11 +208,11 @@ export class OneStorage {
/**
* @brief A map of ConfigObj (key: cfg path)
*/
private _cfgToCfgObjMap: ConfigObjMap;
private _cfgToCfgObjMap: CfgToCfgObjMap;
/**
* @brief A map of BaseModel path to Cfg path
*/
private _baseModelToCfgsMap: StringListMap;
private _baseModelToCfgsMap: BaseModelToCfgMap;

/**
* Get the list of .cfg files within the workspace
Expand Down Expand Up @@ -101,49 +247,17 @@ export class OneStorage {
}
}

private _initCfgToCfgObjMap(cfgList: string[]): ConfigObjMap {
let map: ConfigObjMap = {};

cfgList.forEach(cfg => {
const cfgObj = ConfigObj.createConfigObj(vscode.Uri.file(cfg));
if (cfgObj) {
map[cfg] = cfgObj;
}
});

return map;
}

private _initBaseModelToCfgsMap(cfgList: string[], cfgToCfgObjMap: ConfigObjMap): StringListMap {
let map: StringListMap = {};

cfgList.forEach(cfg => {
const cfgObj = cfgToCfgObjMap[cfg];
if (cfgObj) {
cfgObj.getBaseModelsExists.forEach(baseModelArtifact => {
if (!map[baseModelArtifact.path]) {
map[baseModelArtifact.path] = [];
}

if (!map[baseModelArtifact.path].includes(cfg)) {
map[baseModelArtifact.path].push(cfg);
}
});
}
});

return map;
}

private static _delete(node: Node) {
OneStorage.get()._nodeMap.delete(node.path);
const instance = OneStorage.get();
instance._nodeMap.delete(node.path);

switch (node.type) {
case NodeType.baseModel:
OneStorage.resetBaseModel(node.path);
instance._baseModelToCfgsMap.reset(node.type, node.path);
break;
case NodeType.config:
OneStorage.resetConfig(node.path);
instance._baseModelToCfgsMap.reset(node.type, node.path);
instance._cfgToCfgObjMap.reset(node.type, node.path);
break;
default:
break;
Expand All @@ -152,8 +266,12 @@ export class OneStorage {

private constructor() {
const cfgList = this._getCfgList();
this._cfgToCfgObjMap = this._initCfgToCfgObjMap(cfgList);
this._baseModelToCfgsMap = this._initBaseModelToCfgsMap(cfgList, this._cfgToCfgObjMap);

this._cfgToCfgObjMap = new CfgToCfgObjMap();
this._cfgToCfgObjMap.init(cfgList);

this._baseModelToCfgsMap = new BaseModelToCfgMap();
this._baseModelToCfgsMap.init(cfgList, this._cfgToCfgObjMap);
}

private static _obj: OneStorage|undefined;
Expand All @@ -162,20 +280,20 @@ export class OneStorage {
* Get cfg lists which refers the base model path
* @param baseModelPath
* @return a list of cfg path or undefined
* 'undefined' is returned when
* An empty array is returned when
* (1) the path not exists
* (2) the path is not a base model file
* (3) the path is a lonely base model file
*/
public static getCfgs(baseModelPath: string): string[]|undefined {
return OneStorage.get()._baseModelToCfgsMap[baseModelPath];
return OneStorage.get()._baseModelToCfgsMap.get(baseModelPath);
}

/**
* Get cfgObj from the map
*/
public static getCfgObj(cfgPath: string): ConfigObj|undefined {
return OneStorage.get()._cfgToCfgObjMap[cfgPath];
return OneStorage.get()._cfgToCfgObjMap.get(cfgPath);
}

/**
Expand All @@ -188,7 +306,7 @@ export class OneStorage {
public static insert(node: Node) {
// NOTE
// Only _nodeMap is built by calling this function
// _baseModelToCfgsMap and _cfgToCfgObjMap are built at once by scanning the file system.
// _baseModelToCfgsMap and _cfgToCfgObjMap are built at constuctors
OneStorage.get()._nodeMap.set(node.path, node);
}

Expand Down Expand Up @@ -226,18 +344,4 @@ export class OneStorage {
public static reset(): void {
OneStorage._obj = undefined;
}

public static resetBaseModel(path: string): void {
delete OneStorage.get()._baseModelToCfgsMap[path];
Logger.debug('OneStorage', `Base Mode Path(${path}) is removed.`);
}

public static resetConfig(path: string): void {
delete OneStorage.get()._cfgToCfgObjMap[path];
Object.entries(OneStorage.get()._baseModelToCfgsMap).forEach(([modelpath]) => {
OneStorage.get()._baseModelToCfgsMap[modelpath] =
OneStorage.get()._baseModelToCfgsMap[modelpath].filter(cfg => cfg !== path);
});
Logger.debug('OneStorage', `Config Path(${path}) is removed.`);
}
}
Loading