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.

Sign up for GitHub

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

Jump to bottom

Fix completion after xref: for old double-square bracket notation #667

Merged
merged 1 commit into from
Nov 26, 2022
Merged

Fix completion after xref: for old double-square bracket notation #667

merged 1 commit into from
Nov 26, 2022

Conversation

apupier
Copy link
Member

@apupier apupier commented Nov 18, 2022 •
edited
Loading

part of #648

  • it was broken during enforcing code style, the regular expression was not correctly converted
    cd477c7
  • added a test
  • Maybe not very nice in the sense that it is not inserting the file path but only the label, but maybe it is enough if there is the right include?

image

@apupier
Fix completion after xref: for old double-square bracket notation
f09d2f7 
part of asciidoctor#648

- it was broken during enforcing code style, the regular expression was
not correctly converted
asciidoctor@cd477c7
- added a test
- The previous was not very nice in the sense that it is not inserting
the file path but only the label, but maybe it is enough if there is the
right include?

Signed-off-by: Aurélien Pupier <apupier@redhat.com>
apupier
apupier commented Nov 18, 2022
View reviewed changes
src/providers/xref.provider.ts
.map((result) => result.replace('[[', '').replace(']]', ''))
)
const matched = contentOfFilesConcatenated.match(regex)
if (matched) {
Copy link
Member Author

Choose a reason for hiding this comment

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

checking for null, previously there was an exception caught by the VS Code framework.

@apupier apupier mentioned this pull request Nov 18, 2022
Merged
@ggrossetie
Copy link
Member

Maybe not very nice in the sense that it is not inserting the file path but only the label, but maybe it is enough if there is the right include?

We can use insertText on completionItems to provide a text that should be inserted in the document when selection a completion: https://code.visualstudio.com/api/references/vscode-api#CompletionItem

@ggrossetie
Copy link
Member

added a test

THANKS 🙌🏻

@apupier apupier mentioned this pull request Nov 25, 2022
Open
@apupier
Copy link
Member Author

apupier commented Nov 25, 2022

Maybe not very nice in the sense that it is not inserting the file path but only the label, but maybe it is enough if there is the right include?

We can use insertText on completionItems to provide a text that should be inserted in the document when selection a completion: https://code.visualstudio.com/api/references/vscode-api#CompletionItem

My question was if it is even valid to insert it this way currently. But I guess we can split it anyway. it is helping users anyway, and can insert in a second iteration. I mentioned it as a seprate task here #648 (comment)

@apupier
Copy link
Member Author

apupier commented Nov 25, 2022

@Mogztter if fine for you, I think we can merge this Pull Request as-is and iterate to improve the mentioned points.

@ggrossetie ggrossetie merged commit be5055d into asciidoctor:master Nov 26, 2022
@ggrossetie
Copy link
Member

@apupier And merged, thanks!

You will need to rebase your other pull requests 😬

@apupier apupier deleted the 648-fixExistingWithDoubleSquareBracket branch December 23, 2022 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ggrossetie ggrossetie ggrossetie left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@apupier @ggrossetie