Skip to content

accept dash-normalized package name prefix in skill names#10

Open
faithoflifedev wants to merge 1 commit into
serverpod:mainfrom
faithoflifedev:feat/skill-scanner-dash-underscore-prefix
Open

accept dash-normalized package name prefix in skill names#10
faithoflifedev wants to merge 1 commit into
serverpod:mainfrom
faithoflifedev:feat/skill-scanner-dash-underscore-prefix

Conversation

@faithoflifedev
Copy link
Copy Markdown

resolves issue #9 - vs code linter does not accept underscore in skill names

Copy link
Copy Markdown
Contributor

@marcelomendoncasoares marcelomendoncasoares left a comment

Choose a reason for hiding this comment

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

Hi @faithoflifedev!

Thanks for trying to fix this issue! There is, however, a fundamental flaw in the current approach, because it can generate conflicts between package and skill names. Maybe a '---' as separator between the package name and skill is safer in case the package has underscores in the name?

Comment on lines 46 to +61
@@ -53,10 +54,11 @@ class SkillScanner {
final skillMdFile = File(p.join(entity.path, 'SKILL.md'));
if (!await skillMdFile.exists()) continue;

if (!skillName.startsWith(prefix)) {
if (!(skillName.startsWith(prefix) ||
skillName.startsWith(prefixWithDashes))) {
stderr.writeln(
'Warning: Skipping skill "$skillName" in ${package.name} '
'-- name must start with "${package.name}-"',
'-- name must start with "$prefix" or "$prefixWithDashes"',
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This fix is actually dangerous because it becomes impossible to distinguish between the package name and the skill name. A skill named user-auth-login could legitimately belong to:

  1. A package named user_auth with a skill named login (because user_auth becomes user-auth-).
  2. A package named user with a skill named auth-login.

This would break the logic for managing the skills.

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