Skip to content

Commit

Permalink
[Editing Books] - Refactor buildToc method (microsoft#14532)
Browse files Browse the repository at this point in the history 
* refactor buildToc method to only modify the section found and return a boolean

* fix tests

* Address pr comments
  • Loading branch information
@barbaravaldez
barbaravaldez authored Mar 5, 2021
1 parent b82942a commit a17a4a5
Show file tree
Hide file tree
Showing 3 changed files with 71 additions and 91 deletions.
123 changes: 50 additions & 73 deletions extensions/notebook/src/book/bookTocManager.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,22 +30,17 @@ export function hasSections(node: JupyterBookSection): boolean {
}

export class BookTocManager implements IBookTocManager {
public tableofContents: JupyterBookSection[];
public newSection: JupyterBookSection;
private _movedFiles: Map<string, string>;
private _modifiedDirectory: Set<string>;
private _tocFiles: Map<string, JupyterBookSection[]>;
public tableofContents: JupyterBookSection[] = [];
public newSection: JupyterBookSection = {};
public movedFiles: Map<string, string> = new Map<string, string>();
private _modifiedDirectory: Set<string> = new Set<string>();
public tocFiles: Map<string, string> = new Map<string, string>();
private sourceBookContentPath: string;
private targetBookContentPath: string;
private _sourceBook: BookModel;

constructor(targetBook?: BookModel, sourceBook?: BookModel) {
this._sourceBook = sourceBook;
this.newSection = {};
this.tableofContents = [];
this._movedFiles = new Map<string, string>();
this._modifiedDirectory = new Set<string>();
this._tocFiles = new Map<string, JupyterBookSection[]>();
this.sourceBookContentPath = sourceBook?.bookItems[0].rootContentPath;
this.targetBookContentPath = targetBook?.bookItems[0].rootContentPath;
}
Expand Down Expand Up @@ -132,7 +127,7 @@ export class BookTocManager implements IBookTocManager {
counter++;
}
await fs.move(src, path.join(newFileName.concat(' - ', counter.toString())).concat(path.parse(dest).ext), { overwrite: true });
this._movedFiles.set(src, path.join(newFileName.concat(' - ', counter.toString())).concat(path.parse(dest).ext));
this.movedFiles.set(src, path.join(newFileName.concat(' - ', counter.toString())).concat(path.parse(dest).ext));
vscode.window.showInformationMessage(loc.duplicateFileError(path.parse(dest).base, src, newFileName.concat(' - ', counter.toString())));
return newFileName.concat(' - ', counter.toString());
}
Expand All @@ -145,13 +140,14 @@ export class BookTocManager implements IBookTocManager {
* Rewrite the original table of contents of the book, in case of error as well.
*/
async recovery(): Promise<void> {
this._movedFiles.forEach(async (value, key) => {
for (const [key, value] of this.movedFiles.entries()) {
await fs.move(value, key);
});
}

this._tocFiles.forEach(async (value, key) => {
await fs.writeFile(key, yaml.safeDump(value, { lineWidth: Infinity, noRefs: true, skipInvalid: true }));
});
for (const [key, value] of this.tocFiles.entries()) {
const yamlFile = await yaml.safeLoad(value);
await fs.writeFile(key, yaml.safeDump(yamlFile, { lineWidth: Infinity, noRefs: true, skipInvalid: true }));
}
}

async cleanUp(directory: string): Promise<void> {
Expand All @@ -165,7 +161,7 @@ export class BookTocManager implements IBookTocManager {
let fileStat = await fs.stat(filePath);
if (fileStat.isFile) {
//check if the file is in the moved files
let newPath = this._movedFiles.get(filePath);
let newPath = this.movedFiles.get(filePath);
if (newPath) {
let exists = await fs.pathExists(newPath);
// if the file exists in the new path and if the the new path and old path are different
Expand All @@ -190,54 +186,51 @@ export class BookTocManager implements IBookTocManager {
* @param addSection The section that'll be added to the target section. If it's undefined then the target section (findSection) is removed from the table of contents.
*/
async updateTOC(version: BookVersion, tocPath: string, findSection: JupyterBookSection, addSection?: JupyterBookSection): Promise<void> {
const toc = yaml.safeLoad((await fs.readFile(tocPath, 'utf8')));
this._tocFiles.set(tocPath, toc);
let newToc = new Array<JupyterBookSection>(toc.length);
for (const [index, section] of toc.entries()) {
let newSection = this.buildTOC(version, section, findSection, addSection);
if (newSection) {
newToc[index] = newSection;
}
const tocFile = await fs.readFile(tocPath, 'utf8');
this.tableofContents = yaml.safeLoad(tocFile);
if (!this.tocFiles.has(tocPath)) {
this.tocFiles.set(tocPath, tocFile);
}
const isModified = this.modifyToc(version, this.tableofContents, findSection, addSection);
if (isModified) {
await fs.writeFile(tocPath, yaml.safeDump(this.tableofContents, { lineWidth: Infinity, noRefs: true, skipInvalid: true }));
} else {
throw (new Error(loc.sectionNotFound(findSection.title, tocPath)));
}
await fs.writeFile(tocPath, yaml.safeDump(newToc, { lineWidth: Infinity, noRefs: true, skipInvalid: true }));
this.tableofContents = newToc;
}

/**
* Creates a new table of contents structure containing the added section. This method is only called when we move a section to another section.
* Since the sections can be arranged in a tree structure we need to look for the section that will be modified in a recursively.
* Modify the table of contents read from file to add or remove a section.
*
* Returns true if the table of contents is modified successfully. If it does not find the section that will be modified then that means that the toc and book tree view are not showing the same information,
* in that case return false.
*
* @param version Version of the book
* @param section The current section that we are iterating
* @param toc The table of contents that will be modified
* @param findSection The section that will be modified.
* @param addSection The section that'll be added to the target section. If it's undefined then the target section (findSection) is removed from the table of contents.
*/
private buildTOC(version: BookVersion, section: JupyterBookSection, findSection: JupyterBookSection, addSection: JupyterBookSection): JupyterBookSection {
// condition to find the section to be modified
if (section.title === findSection.title && (section.file && section.file === findSection.file || section.url && section.url === findSection.file)) {
if (addSection) {
//if addSection is not undefined, then we added to the table of contents.
section.sections !== undefined && section.sections.length > 0 ? section.sections.push(addSection) : section.sections = [addSection];
return section;
}
// if addSection is undefined then we remove the whole section from the table of contents.
return addSection;
} else {
let newSection = convertTo(version, section);
if (section.sections && section.sections.length > 0) {
newSection.sections = [] as JupyterBookSection[];
for (let s of section.sections) {
const child = this.buildTOC(version, s, findSection, addSection);
if (child) {
newSection.sections.push(convertTo(version, child));
}
private modifyToc(version: BookVersion, toc: JupyterBookSection[], findSection: JupyterBookSection, addSection: JupyterBookSection): boolean {
for (const [index, section] of toc.entries()) {
if (section.title === findSection.title && (section.file && section.file === findSection.file || section.url && section.url === findSection.file)) {
if (addSection) {
//if addSection is not undefined, then we added to the table of contents.
section.sections !== undefined && section.sections.length > 0 ? section.sections.push(addSection) : section.sections = [addSection];
toc[index] = section;
} else {
// if addSection is undefined then we remove the whole section from the table of contents.
toc[index] = addSection;
}
return true;
} else if (hasSections(section)) {
const found = this.modifyToc(version, section.sections, findSection, addSection);
if (found) {
// we only want to return true if it finds the section and modifies the toc
return true;
}
}
if (newSection.sections?.length === 0) {
// if sections is an empty array then assign it to undefined, so it's converted into a markdown file.
newSection.sections = undefined;
}
return newSection;
}
return false;
}

/**
Expand Down Expand Up @@ -278,7 +271,7 @@ export class BookTocManager implements IBookTocManager {
let fileName = undefined;
try {
await fs.move(path.join(this.sourceBookContentPath, elem.file).concat('.ipynb'), path.join(this.targetBookContentPath, elem.file).concat('.ipynb'), { overwrite: false });
this._movedFiles.set(path.join(this.sourceBookContentPath, elem.file).concat('.ipynb'), path.join(this.targetBookContentPath, elem.file).concat('.ipynb'));
this.movedFiles.set(path.join(this.sourceBookContentPath, elem.file).concat('.ipynb'), path.join(this.targetBookContentPath, elem.file).concat('.ipynb'));
} catch (error) {
if (error.code === 'EEXIST') {
fileName = await this.renameFile(path.join(this.sourceBookContentPath, elem.file).concat('.ipynb'), path.join(this.targetBookContentPath, elem.file).concat('.ipynb'));
Expand All @@ -289,7 +282,7 @@ export class BookTocManager implements IBookTocManager {
}
try {
await fs.move(path.join(this.sourceBookContentPath, elem.file).concat('.md'), path.join(this.targetBookContentPath, elem.file).concat('.md'), { overwrite: false });
this._movedFiles.set(path.join(this.sourceBookContentPath, elem.file).concat('.md'), path.join(this.targetBookContentPath, elem.file).concat('.md'));
this.movedFiles.set(path.join(this.sourceBookContentPath, elem.file).concat('.md'), path.join(this.targetBookContentPath, elem.file).concat('.md'));
} catch (error) {
if (error.code === 'EEXIST') {
fileName = await this.renameFile(path.join(this.sourceBookContentPath, elem.file).concat('.md'), path.join(this.targetBookContentPath, elem.file).concat('.md'));
Expand Down Expand Up @@ -319,7 +312,7 @@ export class BookTocManager implements IBookTocManager {
let fileName = undefined;
try {
await fs.move(section.book.contentPath, path.join(this.targetBookContentPath, moveFile).concat(path.parse(uri).ext), { overwrite: false });
this._movedFiles.set(section.book.contentPath, path.join(this.targetBookContentPath, moveFile).concat(path.parse(uri).ext));
this.movedFiles.set(section.book.contentPath, path.join(this.targetBookContentPath, moveFile).concat(path.parse(uri).ext));
} catch (error) {
if (error.code === 'EEXIST') {
fileName = await this.renameFile(section.book.contentPath, path.join(this.targetBookContentPath, moveFile).concat(path.parse(uri).ext));
Expand Down Expand Up @@ -438,26 +431,10 @@ export class BookTocManager implements IBookTocManager {
}
}

public get movedFiles(): Map<string, string> {
return this._movedFiles;
}

public get originalToc(): Map<string, JupyterBookSection[]> {
return this._tocFiles;
}

public get modifiedDir(): Set<string> {
return this._modifiedDirectory;
}

public set movedFiles(files: Map<string, string>) {
this._movedFiles = files;
}

public set originalToc(files: Map<string, JupyterBookSection[]>) {
this._tocFiles = files;
}

public set modifiedDir(files: Set<string>) {
this._modifiedDirectory = files;
}
Expand Down
2 changes: 2 additions & 0 deletions extensions/notebook/src/common/localizedConstants.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ export function closeBookError(resource: string, error: string): string { return
export function duplicateFileError(title: string, path: string, newPath: string): string { return localize('duplicateFileError', "File {0} already exists in the destination folder {1} \n The file has been renamed to {2} to prevent data loss.", title, path, newPath); }
export function editBookError(path: string, error: string): string { return localize('editBookError', "Error while editing book {0}: {1}", path, error); }
export function selectBookError(error: string): string { return localize('selectBookError', "Error while selecting a book or a section to edit: {0}", error); }
export function sectionNotFound(section: string, tocPath: string): string { return localize('sectionNotFound', "Failed to find section {0} in {1}.", section, tocPath); }

// Remote Book dialog constants
export const url = localize('url', "URL");
Expand Down Expand Up @@ -97,3 +98,4 @@ export const msgSaveFolderError = localize('msgSaveFolderError', "Save location
export function msgCreateBookWarningMsg(file: string): string { return localize('msgCreateBookWarningMsg', "Error while trying to access: {0}", file); }



37 changes: 19 additions & 18 deletions extensions/notebook/src/test/book/bookTocManager.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import { BookTreeViewProvider } from '../../book/bookTreeView';
import { NavigationProviders } from '../../common/constants';
import * as loc from '../../common/localizedConstants';


export function equalTOC(actualToc: IJupyterBookSectionV2[], expectedToc: IJupyterBookSectionV2[]): boolean {
for (let [i, section] of actualToc.entries()) {
if (section.title !== expectedToc[i].title || section.file !== expectedToc[i].file) {
Expand Down Expand Up @@ -95,7 +96,7 @@ describe('BookTocManagerTests', function () {
}];
await bookTocManager.createBook(bookFolderPath, root2FolderPath);
should(equalTOC(bookTocManager.tableofContents[1].sections, expectedSection)).be.true;
should(bookTocManager.tableofContents[1].file).be.equal(path.join(path.sep, subfolder, 'index'));
should((bookTocManager.tableofContents[1] as IJupyterBookSectionV2).file).be.equal(path.join(path.sep, subfolder, 'index'));
});

it('should ignore invalid file extensions', async () => {
Expand Down Expand Up @@ -338,7 +339,11 @@ describe('BookTocManagerTests', function () {
treeItemCollapsibleState: undefined,
type: BookTreeItemType.Markdown,
version: run.version,
page: run.sectionA.sectionFormat
page: {
title: run.sectionA.sectionName,
file: path.join(path.sep, 'sectionA', 'readme'),
sections: run.sectionA.sectionFormat
}
};

// section B is from source book
Expand All @@ -353,7 +358,11 @@ describe('BookTocManagerTests', function () {
treeItemCollapsibleState: undefined,
type: BookTreeItemType.Markdown,
version: run.version,
page: run.sectionB.sectionFormat
page: {
title: run.sectionB.sectionName,
file: path.join(path.sep, 'sectionB', 'readme'),
sections: run.sectionB.sectionFormat
}
};

// notebook5 is from source book
Expand All @@ -374,12 +383,8 @@ describe('BookTocManagerTests', function () {
type: BookTreeItemType.Notebook,
version: run.version,
page: {
sections: [
{
'title': 'Notebook 5',
'file': path.join(path.sep, 'notebook5')
}
]
'title': 'Notebook 5',
'file': path.join(path.sep, 'notebook5')
}
};

Expand All @@ -400,12 +405,8 @@ describe('BookTocManagerTests', function () {
type: BookTreeItemType.Notebook,
version: run.version,
page: {
sections: [
{
'title': 'Notebook 5',
'file': path.join(path.sep, 'notebook5')
}
]
'title': 'Notebook 5',
'file': path.join(path.sep, 'notebook5')
}
};

Expand All @@ -432,7 +433,7 @@ describe('BookTocManagerTests', function () {
sectionA.tableOfContentsPath = run.sourceBook.tocPath;
sectionB.tableOfContentsPath = run.sourceBook.tocPath;
notebook.tableOfContentsPath = run.sourceBook.tocPath;
duplicatedNotebook.tableOfContentsPath = run.sourceBook.tocPath;
duplicatedNotebook.tableOfContentsPath = undefined;

sectionA.sections = run.sectionA.sectionFormat;
sectionB.sections = run.sectionB.sectionFormat;
Expand Down Expand Up @@ -468,9 +469,9 @@ describe('BookTocManagerTests', function () {
}

// target book
await fs.writeFile(run.targetBook.tocPath, '- title: Welcome\n file: /readme\n- title: Section C\n file: /sectionC/readme\n sections:\n - title: Notebook6\n file: /sectionC/notebook6');
await fs.writeFile(run.targetBook.tocPath, '- title: Welcome\n file: /readme\n- title: Section C\n file: /sectionC/readme\n sections:\n - title: Notebook 6\n file: /sectionC/notebook6');
// source book
await fs.writeFile(run.sourceBook.tocPath, '- title: Notebook 5\n file: /notebook5\n- title: Section A\n file: /sectionA/readme\n sections:\n - title: Notebook1\n file: /sectionA/notebook1\n - title: Notebook2\n file: /sectionA/notebook2');
await fs.writeFile(run.sourceBook.tocPath, '- title: Notebook 5\n file: /notebook5\n- title: Section A\n file: /sectionA/readme\n sections:\n - title: Notebook1\n file: /sectionA/notebook1\n - title: Notebook2\n file: /sectionA/notebook2\n- title: Section B\n file: /sectionB/readme\n sections:\n - title: Notebook3\n file: /sectionB/notebook3\n - title: Notebook4\n file: /sectionB/notebook4');

const mockExtensionContext = new MockExtensionContext();

Expand Down

0 comments on commit a17a4a5

Please sign in to comment.