Hi @byronluk, wow, thank you for looking at this - I appreciate it greatly.

And thank you for taking care with the test cases.

Before looking at the code, I did some exploratory testing - I spent about 45 minutes trying to come up with cases to fool it.

Summary of thoughts after exploratory testing

1. In moderately deeply nested vaults, I think users may grumble at the new rendering of the whole file path in links.
2. The new code doesn't fix links that have have display/alias text after | inside the link
3. I don't think using a regular expression is going to be workable, realistically
   • in testing, it is too easy to trick it, and make it change text which looks like a link but is not
   • so it will break display results for users in real-world cases

I don't know how experienced you are with the Obsidian API?
If not familiar, and if you are interested in refining this PR to use Obsidian's parsing of links, I think we should probably have a chat before you start on it... My email address is in my profile here, if you would like to set up a virtual meeting some time soon....

Cases not tested

• I haven't yet test []() style links - only [[]]

How I tested

These are the files I used:

• 3297 - Search.md
• 3297 - Tasks.md

3297 - Search.md

# 3297 - links in descriptions - Search

```tasks
folder includes {{query.file.folder}}

sort by function task.lineNumber
group by heading

hide backlinks
```

3297 - Tasks.md

# 3297 - links in descriptions - Tasks

## 1 Works before and after the PR

- [ ] #task ✅ Simple case - single link to current file name and heading <br>[[3297 - Tasks#1 Works before and after the PR]]

## 2 Only works after the PR

... but should it display the whole path?

I believe that the Tasks plugin only puts the full path in to its "backlinks" if the filename isn't unique in the vault.

Should this code do the same?

- [ ] #task ✅ Simple case - single relative link to heading in current file <br>[[#2 Only works after the PR]]
- [ ] #task ✅ Simple case - two relative link to heading in current file - both are updated <br>[[#2 Only works after the PR]]<br>[[#3297 - links in descriptions - Tasks]]

## 3 What I think the new behaviour should possibly look like

- [ ] #task ✅ Simple case - single  link <br>[[3297 - Tasks#3 What I think the new behaviour should possibly look like|#3 What I think the new behaviour should possibly look like]]

## 4 Does not work before or after PR

It appears that the regular expression does not allow for text like `|I am an alias`  to set display text of a link.

- [ ] #task ❌ Simple case - single relative link to heading in current file - with alias <br>[[#4 Does not work before or after PR|I am an alias]]

## 5 Should not be modified by PR

The regular expression is modifying that Obsidian does not render as a link in Reading mode, in Restricted mode.

- [ ] #task ❌ Formatted text looks like a link but should not be modified `[[#5 Should not be modified by PR]]`
- [ ] #task ❌ Escaped `[` should not be modified \[[#5 Should not be modified by PR]]

We don't want to be writing Markdown-parsing code in Regexes.

Instead we should be using the [LinkCache](https://docs.obsidian.md/Reference/TypeScript+API/LinkCache).

There are some examples of the LinkCache context in Tasks test data: [link_in_task_wikilink.json](https://github.com/obsidian-tasks-group/obsidian-tasks/blob/c6cb8ddc5491d36e48b183b1c38786be09504103/tests/Obsidian/__test_data__/link_in_task_wikilink.json#L21-L56).

Here is what the query looks like, running the code in the PR:

It would also be worth testing rendering of nested list items that contain links to headings in their own file - and when their parent task line is shown in search results that contain show tree.

It would also be worth testing rendering of nested list items that contain links to headings in their own file - and when their parent task line is shown in search results that contain show tree.

At the moment list item and task descriptions are drawn by different code - @ilandikov and I are soon going to start unifying those two bodies of code.

So it would be fine for this PR to focus just on fixing task lines - and then we can later make sure that any fix is also applied to list items.

@byronluk many thanks for looking at this and spending time for this PR. I left a couple of suggestions on code level.

@claremacrae it would be nice to refactor this is in a way to make this change easyER (I don't find it not_easy). Maybe we can discuss this on our next session before refactoring the description rendering you mentioned before. In any case I agree with you that this could be merged without the latter.

@byronluk many thanks for looking at this and spending time for this PR. I left a couple of suggestions on code level.

Thanks...

@claremacrae it would be nice to refactor this is in a way to make this change easyER (I don't find it not_easy). Maybe we can discuss this on our next session before refactoring the description rendering you mentioned before. In any case I agree with you that this could be merged without the latter.

Where I haven't already paired with contributors, I don't request the 'make the change easy/make the easy change' approach, as the technique is just not sufficiently well known, and most people don't use refactoring IDEs either... It's a skill that takes quite some time to learn...

So I focus on small PRs, and ensuring good exploratory testing and agreeing the desired behaviour before giving feedback on code.

Thanks for the detailed feedback @claremacrae, @ilandikov! I agreed, I'm not a huge fan of the extra parameter I introduced either 😅. I'm not very familiar with the codebase and wanted to timebox, so figured it'd be best to put a PR out where we could anchor our discussion around.

I don't know how experienced you are with the Obsidian API?
If not familiar, and if you are interested in refining this PR to use Obsidian's parsing of links, I think we should probably have a chat before you start on it... My email address is in my profile here, if you would like to set up a virtual meeting some time soon....

This sounds like a great idea! I'll poke around and see if we can resolve this async, as I'm quite busy with work and my 1 year old haha..

I'll take a look at:

1. Finding a better solution that doesn't involve using regex (will look into using Obsidian's parsing of links like @claremacrae mentioned)
2. Avoidng the extra parameter introduced into the constructor,
3. The thorough test cases provided.

Will keep you all updated on my progress here! Probably should be able to get around to this within the next week or so.

Super - thanks very much @byronluk.

Given your time constraints, I suggest you don’t worry about the parameter, though. We can always refactor the code easily after the PR is merged.

PS @byronluk ...

I like your mentioning of time-boxing...

Feel free to time-box your next investigations, and then send me an email to have a chat.

Because of my familiarity with the code, I can see the next few steps on implementation and testing and it will save you hours, whilst still leaving you the actual problem to fix. :-)

Plan B: If you really, really don't have time for a chat, let me know if you are happy for me to push some demo code to your PR branch instead - and I'll explain things in comments instead.

It would be lower band-width communication, but might fit in better with your schedule...

And could be followed by a later conversation, too, if need be...

@claremacrae, @ilandikov I took another pass at the change using the linkCache. Would appreciate your feedback!

I went through the test cases you provided @claremacrae and this change should cover them all correctly

I'll take another pass through when I have some time tomorrow! Will fix up the simple mistakes (sorry about the messy test cases!) and think about the positioning problem.

Thank you for the thorough review!

I'll take another pass through when I have some time tomorrow! Will fix up the simple mistakes (sorry about the messy test cases!) and think about the positioning problem.

Thank you for the thorough review!

Thank you for this reply, but....

tl;dr I have been thinking about it quite a lot, and I actively do not want you to adjust this code to address the positioning problem! 😄

Rather than having the rendering code try to find links, I feel the the right-or-sustainable answer is to make links first-class objects in the Tasks code, and to use the stored link positions in the rendering.

So, please do not spend your time making the current PR use Obsidian's link-location data.

As well as making the current code a lot more complicated, for what would be an interim measure, the writing of tests for it would be nightmarishly complicated. Here are some of the scenarios to consider:

• No Global Filter:
• Global filter set:
  • and hidden in search results
  • and shown in search results
  • in the middle of the line
  • at the end of the line
• Extra whitespace
  • at beginning of line
  • end of line
• Position of links
  • After signifiers (invalid task line, but still useful test)
• etc etc

Let's instead focus on getting the current implementation ready to merge (fix the misleading comment, improve the tests)...

Rather than having the rendering code try to find links, I feel the the right-or-sustainable answer is to make links first-class objects in the Tasks code, and to use the stored link positions in the rendering.

I agree with this :) I did fix up my solution before seeing your comment and I do think it is a fairly reasonable approach with minimal added complexity- I'll push it up and see what you think and happy to revert back to the current approach we have here if you'd like.

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud