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

Parse tags starting with digits and containing spaces (close #1072) #1116

Merged
merged 1 commit into from
Oct 20, 2024

Conversation

Maarrk
Copy link
Contributor

@Maarrk Maarrk commented Oct 14, 2024

I'd suggest to continue discussion in #1072

@Maarrk Maarrk marked this pull request as draft October 14, 2024 18:40
@Maarrk Maarrk marked this pull request as ready for review October 15, 2024 22:54
@Maarrk Maarrk changed the title WIP: Parse tags starting with digits and containing spaces (close #1072) Parse tags starting with digits and containing spaces (close #1072) Oct 16, 2024
Copy link
Collaborator

@zefhemel zefhemel left a comment

Choose a reason for hiding this comment

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

Added a few comments, but looking forward to get this in!

plug-api/lib/tags.ts Outdated Show resolved Hide resolved
common/markdown_parser/parser.ts Outdated Show resolved Hide resolved
@@ -594,6 +594,32 @@ const Hashtag = regexParser(
},
);

/** Extract the name from hashtag text, removing # prefix and <angle brackets> if necessary */
export function hashtagName(text: string): string {
Copy link
Collaborator

Choose a reason for hiding this comment

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

what about extractHashtag?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. There is another function extractHashTags in the same place, which operates on a tree. Should we do anything to prevent confusion? I didn't touch it so far, to not make more changes than necessary

common/markdown_parser/parser.ts Outdated Show resolved Hide resolved
Includes tests and Markdown/Hashtags page for website. Closes silverbulletmd#1072
@Maarrk Maarrk requested a review from zefhemel October 17, 2024 20:52
@@ -4,6 +4,7 @@ import {
type ParseTree,
traverseTree,
} from "@silverbulletmd/silverbullet/lib/tree";
import { tagRegex } from "$common/markdown_parser/constants.ts";
Copy link
Collaborator

@zefhemel zefhemel Oct 19, 2024

Choose a reason for hiding this comment

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

The problem we have now is that this means that $common needs to be defined in any plug's import map relying on this file, which will be the case for all the builtins, but not for external plugs. Two options: either move the constant into plug-api/lib (again: I think this very file tags.ts would be fine) or use a relative path: ../../common/markdown_parser/... instead.

@zefhemel
Copy link
Collaborator

Ok you know what, let me just merge this as it and fix it on main.

@zefhemel zefhemel merged commit 010e2b2 into silverbulletmd:main Oct 20, 2024
1 check passed
@Maarrk
Copy link
Contributor Author

Maarrk commented Oct 20, 2024

OK, thanks! I'll write down the changes I needed to do here for future PRs

@zefhemel
Copy link
Collaborator

I hope you're taking a note in SilverBullet.

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