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

Improve link handling in markdown links #120

Merged
merged 2 commits into from
Aug 1, 2024

Conversation

martin-fleck-at
Copy link

@martin-fleck-at martin-fleck-at commented Jul 31, 2024

What it does

  • Extend EditorManager to have more flexible selection resolution
  • Allow adopters to register selection resolvers
    -- Provide selection resolver for GitHub location syntax
    -- Provide selection resolver for TypeDoc symbol navigation
    -- Provide selection provider for fragment textual search
  • Fix minor issue in editor-manager where URI is not passed properly

Fixes https://github.com/eclipsesource/osweek-2024/issues/18

How to test

  • GitHub Location match: Ensure that you generate a link with file://<path>#<location> where location is L<start-line>C<start-column>-L<end-line>C<end-column> (end position is optional and all columns are optional as well)
  • Symbol match: Ensure that you generate a link with file://<path>#<symbol-path> where symbol path can be just the symbol name (e.g., a function name in TypeScript) or a nested path in the form of parent.child (e.g., a function nested in a namespace). Ensure symbols are actually provided for your document by checking the Outline or triggering the command 'Go to Symbol...` (not Go to Symbol in Workspace...).
  • Any text match: Ensure that you generate a link with file://<path>#<text>

link-support

Follow-ups

Review checklist

Reminder for reviewers

- Extend EditorManager to have more flexible selection resolution
- Allow adopters to register selection resolvers
-- Provide selection resolver for GitHub location syntax
-- Provide selection resolver for TypeDoc symbol navigation
-- Provide selection provider for fragment textual search
- Fix minor issue in editor-manager where URI is not passed properly

Fixes eclipsesource/osweek-2024#18
Copy link

@lucas-koehler lucas-koehler left a comment

Choose a reason for hiding this comment

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

Code looks pretty good to me and works well to me. I have only two tiny remarks/questions inline :)

Comment on lines 153 to 155
const input = uri.toString();
const hashIndex = input.indexOf('#');
return hashIndex === -1 ? undefined : input.substring(hashIndex + 1);

Choose a reason for hiding this comment

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

Maybe I'm missing something but couldn't you just use uri.fragment to get the fragment?

Copy link
Author

Choose a reason for hiding this comment

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

Good catch! I was starting with string manipulation and only later moved it into the editor manager where we already have the URI, thank you!

Comment on lines 105 to 107
const input = uri.toString();
const hashIndex = input.indexOf('#');
return hashIndex === -1 ? undefined : input.substring(hashIndex + 1).split('.');

Choose a reason for hiding this comment

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

Maybe I'm missing something but couldn't you just use uri.fragment to get the fragment?

Copy link
Author

Choose a reason for hiding this comment

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

Same as above, good catch, thank you!

Copy link

@lucas-koehler lucas-koehler left a comment

Choose a reason for hiding this comment

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

LGTM thanks ✨

@martin-fleck-at martin-fleck-at merged commit 61fe8f3 into feat/ai-chat Aug 1, 2024
1 check passed
@sdirix sdirix deleted the feat/18-markdown-links branch August 12, 2024 09:48
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