-
Notifications
You must be signed in to change notification settings - Fork 0
feat(content-item): add tree command, improved circular dependency import #1
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: feature/clone
Are you sure you want to change the base?
Conversation
| owner: RepositoryContentItem; | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| parent: any; |
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.
Use unknown instead of any, it's safer. To use the value of parent you will need to check the type before you can use it, see Type Guards.
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.
Done, though I used the types that the content is normally traversed with.
|
|
||
| item.dependancies.forEach(dep => { | ||
| rewriteDependancy(dep, mapping); | ||
| rewriteDependancy(dep, mapping, pass === 0); |
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.
This is just a comment regarding this entire method, I think its too complex (cyclomatic complexity - there's a lot of loops and if statements). Can this be simplied down into a series of functions, where each one can be unit tested.
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.
Do you mean that the rewriteDependancy method is complex, or that the importTree method is complex? For the latter, it's probably refactoring it in another PR.
src/commands/content-item/tree.ts
Outdated
| const circularPipes = ['╗', '║', '╝']; | ||
| const circularLine = '═'; | ||
|
|
||
| export const printDependency = ( |
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.
Can this function return lines, circularLinks & evalThis to make it clearer that this function manipulates these args.
Perhaps rename the function as technically it doesn't print anything.
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.
evalThis is copied for each recursive call to this method. For the rest of the state, I've made a small class to hold it, and the method is now called addDependency.
src/commands/content-item/tree.ts
Outdated
| return new ContentDependancyTree(contentItems, new ContentMapping()); | ||
| }; | ||
|
|
||
| type CircularLink = [number, number]; |
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.
Maybe change the circularLinks to be an arrary of an object, as it's not clear what each number represents.
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.
Each number is an index in the lines of the tree. Neither of the numbers have any special meaning - they mean the same thing - it's just a pair of two linked indices. I think giving them names like index1, index2 might not be great.
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.
type CircularLinkFrom = number;
type CircularLinkTo = number;
type CircularLink = [CircularLinkFrom, CircularLinkTo];
ca76162 to
f3175fc
Compare
f3175fc to
ed6bac4
Compare
ed6bac4 to
60f9616
Compare
This command allows you to print a dependency tree for any folder of content items on your system. The input directory should contain content items in the same format that the
exportcommand generates. It does not require repo/folder layouts, just that all dependent content is contained within the parent directory.dc-cli content-item tree <dir>This uses the same dependency tree class as the import task, which uses it to import the deepest content items first so that references can be properly resolved. Here, the "levels" of content items are maintained, and the highest level (most layers of dependencies) is printed first, followed by the lower levels that have not been printed yet.
Each item shows the items that it references or links via a tree structure:
When items have been printed before, their name is shown in brackets and their children are not printed. This saves a lot of space on the tree for content items that are reused a lot.
Circular dependencies are a possibility within DC. Like the import task, they are separated from the rest of the tree into a list of "circular dependencies", now ordered by depth to our best ability.
Overlapping circular dependencies with ordering:
This is useful for examining the dependency structure of content that has been created, and identifying potential problems you might encounter when importing content items to a hub. Since circular dependencies are undesirable and sometimes hard to track manually, it's a useful way of identifying them for removal.
This PR also changes the behaviour of importing content with circular dependencies. The circular dependencies are now imported in order of depth first, and should correctly nullify content references that are not yet available. This will not fix required circular dependencies, but it will fix imports for all circular dependencies that do not use required fields.
This PR is built on top of the
feature/clonebranch. It's in this repo for easier review.