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

feat: Add support for multiple authors and update settings UI #395

Closed
Closed
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
25 changes: 25 additions & 0 deletions src/AuthorStrategyFactory.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
import { PlatformType } from './enums/PlatformType';
import {
AuthorStrategy,


} from './strategies/authors/AuthorStrategy';
import { DocusaurusAuthorStrategy } from './strategies/authors/DocusaurusAuthorStrategy';
import { JekyllAuthorStrategy } from './strategies/authors/JekyllAuthorStrategy';
import { DefaultAuthorStrategy } from './strategies/authors/DefaultAuthorStrategy';

export class AuthorStrategyFactory {
static createStrategy(platform: PlatformType, authors: string): AuthorStrategy {
if (!authors) {
return new DefaultAuthorStrategy();
}
switch (platform) {
case PlatformType.Jekyll:
return new JekyllAuthorStrategy();
case PlatformType.Docusaurus:
return new DocusaurusAuthorStrategy();
default:
return new DefaultAuthorStrategy();
}
}
}
Comment on lines +11 to +25
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider refactoring the class into a simple function.

The AuthorStrategyFactory class contains only a single static method. To improve code simplicity and follow functional programming principles, consider refactoring this class into a simple function.

Apply this diff to refactor the class into a function:

-export class AuthorStrategyFactory {
-  static createStrategy(platform: PlatformType, authors: string): AuthorStrategy {
+export function createAuthorStrategy(platform: PlatformType, authors: string): AuthorStrategy {
     if (!authors) {
       return new DefaultAuthorStrategy();
     }
     switch (platform) {
       case PlatformType.Jekyll:
         return new JekyllAuthorStrategy();
       case PlatformType.Docusaurus:
         return new DocusaurusAuthorStrategy();
       default:
         return new DefaultAuthorStrategy();
     }
   }
-}

This change addresses the static analysis warning and simplifies the code structure.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
export class AuthorStrategyFactory {
static createStrategy(platform: PlatformType, authors: string): AuthorStrategy {
if (!authors) {
return new DefaultAuthorStrategy();
}
switch (platform) {
case PlatformType.Jekyll:
return new JekyllAuthorStrategy();
case PlatformType.Docusaurus:
return new DocusaurusAuthorStrategy();
default:
return new DefaultAuthorStrategy();
}
}
}
export function createAuthorStrategy(platform: PlatformType, authors: string): AuthorStrategy {
if (!authors) {
return new DefaultAuthorStrategy();
}
switch (platform) {
case PlatformType.Jekyll:
return new JekyllAuthorStrategy();
case PlatformType.Docusaurus:
return new DocusaurusAuthorStrategy();
default:
return new DefaultAuthorStrategy();
}
}
🧰 Tools
🪛 Biome

[error] 11-25: Avoid classes that contain only static members.

Prefer using simple functions instead of classes with only static members.

(lint/complexity/noStaticOnlyClass)

37 changes: 27 additions & 10 deletions src/FrontMatterConverter.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
import { ObsidianRegex } from './core/ObsidianRegex';
import { Converter } from './core/Converter';
import yaml from 'js-yaml';
import { AuthorStrategyFactory } from './AuthorStrategyFactory';
import { PlatformType } from './enums/PlatformType';
import { AuthorStrategy } from './strategies/authors/AuthorStrategy';

interface FrontMatter {
export interface FrontMatter {
[key: string]: string | undefined;
}

Expand Down Expand Up @@ -64,17 +67,23 @@ export class FrontMatterConverter implements Converter {
private readonly resourcePath: string;
private readonly isEnableBanner: boolean;
private readonly isEnableUpdateFrontmatterTimeOnEdit: boolean;
private readonly authors: string;
private readonly authorStrategy: AuthorStrategy;

constructor(
fileName: string,
resourcePath: string,
isEnableBanner = false,
isEnableUpdateFrontmatterTimeOnEdit = false,
isEnableBanner: boolean = false,
isEnableUpdateFrontmatterTimeOnEdit: boolean = false,
authors: string = '',
platform: PlatformType = PlatformType.Default
) {
this.fileName = fileName;
this.resourcePath = resourcePath;
this.isEnableBanner = isEnableBanner;
this.isEnableUpdateFrontmatterTimeOnEdit = isEnableUpdateFrontmatterTimeOnEdit;
this.authors = authors;
this.authorStrategy = AuthorStrategyFactory.createStrategy(platform, authors);
}

parseFrontMatter(content: string): [FrontMatter, string] {
Expand All @@ -93,20 +102,28 @@ export class FrontMatterConverter implements Converter {
}

const result = convert(
convertImageFrontMatter(
this.isEnableBanner,
this.fileName,
this.resourcePath,
replaceDateFrontMatter(
{ ...frontMatter },
this.isEnableUpdateFrontmatterTimeOnEdit,
this.applyAuthors(
convertImageFrontMatter(
this.isEnableBanner,
this.fileName,
this.resourcePath,
replaceDateFrontMatter(
{ ...frontMatter },
this.isEnableUpdateFrontmatterTimeOnEdit,
),
),
),
);

return join(result, body);
}

private applyAuthors(frontMatter: FrontMatter): FrontMatter {
if (this.authors) {
return this.authorStrategy.applyAuthors(frontMatter, this.authors);
}
return frontMatter;
}
}

function convertImageFrontMatter(
Expand Down
17 changes: 13 additions & 4 deletions src/docusaurus/docusaurus.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@ import { convertFootnotes } from '../FootnotesConverter';
import { convertDocusaurusCallout } from '../CalloutConverter';
import { convertComments } from '../CommentsConverter';
import { Notice, TFile } from 'obsidian';
import { convertFrontMatter } from '../FrontMatterConverter';
import { FrontMatterConverter } from '../FrontMatterConverter';
import DocusaurusSettings from './settings/DocusaurusSettings';

const markPublished = async (plugin: O2Plugin) => {
const filesInReady = getFilesInReady(plugin);
Expand Down Expand Up @@ -39,21 +40,29 @@ const checkPublished = async (plugin: O2Plugin, file: TFile) => {
};

export const convertToDocusaurus = async (plugin: O2Plugin) => {
const settings = plugin.docusaurus as DocusaurusSettings;
// get file name in ready folder
const markdownFiles = await copyMarkdownFile(plugin);

for (const file of markdownFiles) {
const publishedDate = await checkPublished(plugin, file);

const frontMatterConverter = new FrontMatterConverter(
file.name,
settings.resourcePath(),
false,
false,
settings.authors,
settings.platform,
);

const contents: Contents = await plugin.app.vault.read(file);
const result =
convertComments(
convertDocusaurusCallout(
convertFootnotes(
convertWikiLink(
convertFrontMatter(
contents,
),
frontMatterConverter.convert(contents),
),
),
),
Expand Down
5 changes: 5 additions & 0 deletions src/docusaurus/settings/DocusaurusSettings.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,14 @@
import { O2PluginSettings } from '../../settings';
import { DateExtractionPattern } from '../DateExtractionPattern';
import { PlatformType } from '../../enums/PlatformType';
import { AuthorStrategy } from '../../strategies/authors/AuthorStrategy';

export default class DocusaurusSettings implements O2PluginSettings {
docusaurusPath: string;
dateExtractionPattern: string;
authors: string = '';
authorStrategy: AuthorStrategy;
platform: PlatformType = PlatformType.Docusaurus;

targetPath(): string {
return `${this.docusaurusPath}/blog`;
Expand Down
6 changes: 6 additions & 0 deletions src/enums/PlatformType.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
export enum PlatformType {
Copy link
Owner

@songkg7 songkg7 Oct 14, 2024

Choose a reason for hiding this comment

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

in TS, as const is better than enum type. as follows:

export const PlatformType = {
  DEFAULT, DOCUSAURUS, JEKYLL
}

or refer to src/core/ObsidianRegex.ts

Default,
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Clarify the Purpose of PlatformType.Default

The PlatformType.Default enum member is used in src/FrontMatterConverter.ts:

  • platform: PlatformType = PlatformType.Default

Consider renaming it to a more specific name or adding a comment to explain its purpose.

🔗 Analysis chain

Clarify the purpose of the Default enum member.

The Default enum member might be ambiguous. It's not clear what the default platform is or when it would be used.

Could you provide more context on the intended use of the Default member? If it's not necessary, consider removing it. If it is needed, consider renaming it to something more specific or adding a comment to explain its purpose.

To help understand the usage, let's search for its occurrences in the codebase:


Action Required: Please run the following shell script to search for usages of PlatformType.Default in TypeScript files.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for usages of PlatformType.Default
rg --type typescript "PlatformType\.Default"

Length of output: 81


Script:

#!/bin/bash
# Search for usages of PlatformType.Default in TypeScript files
rg "PlatformType\.Default" --glob "*.ts" --glob "*.tsx"

Length of output: 135

Jekyll,
Docusaurus,
// 추가 플랫폼은 여기에 열거할 수 있습니다.
}
2 changes: 2 additions & 0 deletions src/jekyll/chirpy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ export async function convertToChirpy(plugin: O2Plugin) {
settings.jekyllRelativeResourcePath,
settings.isEnableBanner,
settings.isEnableUpdateFrontmatterTimeOnEdit,
settings.authors,
settings.platform,
);
const resourceLinkConverter = new ResourceLinkConverter(
fileName,
Expand Down
4 changes: 4 additions & 0 deletions src/jekyll/settings/JekyllSettings.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,12 @@
import { O2PluginSettings } from '../../settings';
import { PlatformType } from '../../enums/PlatformType';

export default class JekyllSettings implements O2PluginSettings {
private _jekyllPath: string;
private _jekyllRelativeResourcePath: string;
authors: string = '';
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

⚠️ Potential issue

Consider revising the authors property implementation

While the plural name authors aligns with the suggestion to support multiple authors, the current implementation as a string may not be the most suitable for representing multiple authors. Additionally, the property is public, which is inconsistent with the encapsulation pattern used for other properties in this class.

Consider the following improvements:

  1. Change the type to an array of strings: authors: string[] = [];
  2. Make the property private and add getter/setter methods for consistency:
    private _authors: string[] = [];
    
    get authors(): string[] {
      return this._authors;
    }
    
    set authors(value: string[]) {
      this._authors = value;
    }

These changes would better support multiple authors and maintain consistency with the class's existing design patterns.

platform: PlatformType = PlatformType.Jekyll;
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider encapsulating the platform property

The platform property is correctly typed and initialized. However, to maintain consistency with the class's existing design patterns, consider encapsulating this property:

private _platform: PlatformType = PlatformType.Jekyll;

get platform(): PlatformType {
  return this._platform;
}

set platform(value: PlatformType) {
  this._platform = value;
}

This change would align the platform property with the encapsulation pattern used for other properties in this class, improving overall consistency and maintainability.


pathReplacer(year: string, month: string, day: string, title: string): string {
return `${year}-${month}-${day}-${title}.md`;
}
Expand Down
29 changes: 29 additions & 0 deletions src/settings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,13 +57,42 @@ export class O2SettingTab extends PluginSettingTab {
});
this.addJekyllPathSetting();
this.addJekyllRelativeResourcePathSetting();
this.addJekyllAuthorsSettings();

// docusaurus settings
this.containerEl.createEl('h2', {
text: 'Docusaurus',
});
this.addDocusaurusPathSetting();
this.dateExtractionPatternSetting();
this.addDocusaurusAuthorsSettings();
}
private addJekyllAuthorsSettings() {
const jekyllSettings = this.plugin.jekyll as JekyllSettings;
new Setting(this.containerEl)
.setName('Jekyll Authors')
.setDesc('Enter authors separated by commas. For single author, enter only one name.')
.addText(text => text
.setPlaceholder('author1, author2, ...')
.setValue(jekyllSettings.authors)
.onChange(async (value) => {
jekyllSettings.authors = value;
await this.plugin.saveSettings();
}));
}

private addDocusaurusAuthorsSettings() {
const docusaurusSettings = this.plugin.docusaurus as DocusaurusSettings;
new Setting(this.containerEl)
.setName('Docusaurus Authors')
.setDesc('Enter authors separated by commas. For single author, enter only one name.')
.addText(text => text
.setPlaceholder('author1, author2, ...')
.setValue(docusaurusSettings.authors)
.onChange(async (value) => {
docusaurusSettings.authors = value;
await this.plugin.saveSettings();
}));
Comment on lines +70 to +95
Copy link
Owner

Choose a reason for hiding this comment

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

👍

}

private enableUpdateFrontmatterTimeOnEditSetting() {
Expand Down
6 changes: 6 additions & 0 deletions src/strategies/authors/AuthorStrategy.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
import { FrontMatter } from '../../FrontMatterConverter';

export interface AuthorStrategy {
applyAuthors(frontMatter: FrontMatter, authors: string): FrontMatter;
}

8 changes: 8 additions & 0 deletions src/strategies/authors/DefaultAuthorStrategy.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import { AuthorStrategy } from './AuthorStrategy';
import { FrontMatter } from '../../FrontMatterConverter';

export class DefaultAuthorStrategy implements AuthorStrategy {
applyAuthors(frontMatter: FrontMatter, authors: string): FrontMatter {
return frontMatter;
}
Comment on lines +5 to +7
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

applyAuthors method doesn't use the authors parameter

The current implementation of applyAuthors doesn't utilize the authors parameter, which defeats the purpose of having a default author strategy.

Update the method to use the authors parameter as demonstrated in the previous comment. Additionally, consider handling multiple authors as suggested in the PR comments:

applyAuthors(frontMatter: FrontMatter, authors: string | string[]): FrontMatter {
  if (!frontMatter.authors) {
    const authorList = Array.isArray(authors) ? authors : [authors];
    return { ...frontMatter, authors: authorList };
  }
  return frontMatter;
}

This implementation supports both single and multiple default authors, aligning with Docusaurus's use of plural 'authors'.

}
Comment on lines +1 to +8
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Implementation doesn't fulfill the PR objectives

The current implementation of DefaultAuthorStrategy doesn't align with the PR objectives to set a default author. The applyAuthors method should modify the frontMatter to include the default author information.

Consider updating the applyAuthors method to add the author information to the frontMatter. Here's a suggested implementation:

applyAuthors(frontMatter: FrontMatter, authors: string): FrontMatter {
  if (!frontMatter.authors && authors) {
    return { ...frontMatter, authors: [authors] };
  }
  return frontMatter;
}

This implementation adds the default author to the frontMatter if no authors are present.

15 changes: 15 additions & 0 deletions src/strategies/authors/DocusaurusAuthorStrategy.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
import { AuthorStrategy } from './AuthorStrategy';
import { FrontMatter } from '../../FrontMatterConverter';

export class DocusaurusAuthorStrategy implements AuthorStrategy {
applyAuthors(frontMatter: FrontMatter, authors: string): FrontMatter {
const authorsList = authors.split(',').map(author => author.trim());
delete frontMatter.author;
songkg7 marked this conversation as resolved.
Show resolved Hide resolved
if (authorsList.length === 1) {
frontMatter.authors = authorsList[0];
} else {
frontMatter.authors = `[${authorsList.join(', ')}]`;
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Reconsider the representation of multiple authors for Docusaurus.

The current implementation encloses multiple authors in square brackets as a string. However, Docusaurus typically expects an array of strings for multiple authors.

Consider modifying the implementation to use an actual array:

-      frontMatter.authors = `[${authorsList.join(', ')}]`;
+      frontMatter.authors = authorsList;

This change ensures that the authors field is correctly formatted for Docusaurus.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
frontMatter.authors = `[${authorsList.join(', ')}]`;
frontMatter.authors = authorsList;

}
return frontMatter;
}
}
15 changes: 15 additions & 0 deletions src/strategies/authors/JekyllAuthorStrategy.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
import { AuthorStrategy } from './AuthorStrategy';
import { FrontMatter } from '../../FrontMatterConverter';

export class JekyllAuthorStrategy implements AuthorStrategy {
applyAuthors(frontMatter: FrontMatter, authors: string): FrontMatter {
const authorsList = authors.split(',').map(author => author.trim());
delete frontMatter.authors;
if (authorsList.length === 1) {
frontMatter.author = authorsList[0];
} else {
frontMatter.authors = `[${authorsList.join(', ')}]`;
}
return frontMatter;
}
Comment on lines +5 to +14
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Consider revising the authors property format for Jekyll compatibility.

The implementation handles both single and multiple authors correctly. However, the format for multiple authors might not be compatible with Jekyll's expected format.

Jekyll typically expects multiple authors to be in a YAML array format. Consider modifying line 11 to:

- frontMatter.authors = `[${authorsList.join(', ')}]`;
+ frontMatter.authors = authorsList;

This change ensures that the authors property is set as an array, which is more compatible with Jekyll's YAML front matter parsing.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
applyAuthors(frontMatter: FrontMatter, authors: string): FrontMatter {
const authorsList = authors.split(',').map(author => author.trim());
delete frontMatter.authors;
if (authorsList.length === 1) {
frontMatter.author = authorsList[0];
} else {
frontMatter.authors = `[${authorsList.join(', ')}]`;
}
return frontMatter;
}
applyAuthors(frontMatter: FrontMatter, authors: string): FrontMatter {
const authorsList = authors.split(',').map(author => author.trim());
delete frontMatter.authors;
if (authorsList.length === 1) {
frontMatter.author = authorsList[0];
} else {
frontMatter.authors = authorsList;
}
return frontMatter;
}
🧰 Tools
🪛 Biome

[error] 7-7: Avoid the delete operator which can impact performance.

Unsafe fix: Use an undefined assignment instead.

(lint/performance/noDelete)

}
Loading