Skip to content

#196 Icon Shortcodes plugin support#197

Open
d9k wants to merge 1 commit intodarlal:masterfrom
d9k:master
Open

#196 Icon Shortcodes plugin support#197
d9k wants to merge 1 commit intodarlal:masterfrom
d9k:master

Conversation

@d9k
Copy link
Copy Markdown

@d9k d9k commented Mar 3, 2025

@darlal
Copy link
Copy Markdown
Owner

darlal commented May 24, 2025

Thanks for this PR. It looks like the PR checks didn't run automatically, I'm sorry about that (should be fixed now). The checks would have provided some signal and early feedback on the errors that need to be addressed. As is, this branch fails to even build due to a number of errors. You can run the build locally using npm run build, the same errors should be visible so they can be addressed locally, additionally you be sure to run the tests locally as well using npm run test. Thanks!

Copy link
Copy Markdown
Owner

@darlal darlal left a comment

Choose a reason for hiding this comment

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

It looks like the "Icon Shortcodes" Obsidian plugin still needs to be install separately for this to work. If that's the case it would be preferable to place this feature behind a setting that individual users can enable on their own if they want it, and inform them of this requirement. Updating the Readame with that information would be great as well.

Separately, it would be great to cover these changes with tests.

rules: {
'@typescript-eslint/unbound-method': 'error',
'no-unused-vars': 'off',
'@typescript-eslint/no-unsafe-assignment': 'warn',
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

It would be preferable to avoid changing the severity of this rule. Using any disables many type checking rules and changing this severity will lead to a lot of backsliding and build up of these errors/warnings.

"typescript": "^5.4.5"
},
"dependencies": {
"@aidenlx/obsidian-icon-shortcodes": "0.9.0",
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

It seems like this project may be inactive/unmaintained, the last commit was from October 2022..?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants