Skip to content

Fix: Match links to a single file with their dot extensions#157

Merged
nathonius merged 6 commits intonathonius:mainfrom
joshleaves:fix/156-linking-to-file
Jan 19, 2025
Merged

Fix: Match links to a single file with their dot extensions#157
nathonius merged 6 commits intonathonius:mainfrom
joshleaves:fix/156-linking-to-file

Conversation

@joshleaves
Copy link
Contributor

Tentative fix for #156 .

I thought the issue came from url-parse.ts but nothing in the code was wrong, so I went up the chain, until I found that the initial decorator matcher was stopping at whitespace, commas,...and the dot character. The matcher was always sending a truncated path to parse-url.

Removing it and testing locally got me the expected result.

# Now it behaves as expected:

https://github.com/nathonius/obsidian-github-link/blob/main/src/github/url-parse.ts

image

And of course, the href value is the expected one.

@github-advanced-security
Copy link

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

@nathonius
Copy link
Owner

Apologies for the delay, it's been a very busy time for me. I'll try and look at this before the new year.

@joshleaves
Copy link
Contributor Author

No problem, hope you're taking enough rest for the year's end. And thanks again for all your work on the extension

@nathonius
Copy link
Owner

nathonius commented Dec 30, 2024

So this will work to fix urls that are a file path, but it will cause problems if used within a sentence, for example:

Created issue for this: https://github.com/nathonius/obsidian-github-link/issues/156.

So I think we need to be smarter about this, but I'm not sure how. 🤔

I played around with the regex for a while and came up with this:

https://regex101.com/r/bXPVza/1
/(?<!]\()https:\/\/github\.com\/([A-z\d\-_~+#%&=*@?\/]*(.[\S\n])*)[^\n\s]/g

After this PR goes in I think I'll add some unit tests for various URLs that should and should not match and refine the regex.

@joshleaves
Copy link
Contributor Author

I think I'll add some unit tests for various URLs that should and should not match and refine the regex

I'd like to work on that, since now the brain teaser got to me =D

@nathonius
Copy link
Owner

I think I'll add some unit tests for various URLs that should and should not match and refine the regex

I'd like to work on that, since now the brain teaser got to me =D

well... I don't have the framework set up yet, and that's something I'd prefer to handle myself 😅 but always happy for contributions otherwise

@joshleaves
Copy link
Contributor Author

I've pushed a new commit that checks multiple GitHub URLs against the matcher Regexp, and played a bit more with the Regexp.

Tested, it works on my machine:
Screenshot 2024-12-30 at 20 47 36

@joshleaves
Copy link
Contributor Author

Simplified the RegExp according to the QualityGate recommendation, and ran a lint on the code.

Runs great even with dots, or many links on a single line!

Screenshot 2024-12-30 at 21 02 56

@joshleaves
Copy link
Contributor Author

I added some more tests to the capture regexp since it was also capturing the first word after the repo.

@sonarqubecloud
Copy link

@nathonius nathonius merged commit 9cdfba0 into nathonius:main Jan 19, 2025
4 checks passed
@nathonius nathonius mentioned this pull request Jan 19, 2025
@nathonius
Copy link
Owner

@all-contributors please add @joshleaves for code and tests

@allcontributors
Copy link
Contributor

@nathonius

I've put up a pull request to add @joshleaves! 🎉

Copy link

@claremacrae claremacrae left a comment

Choose a reason for hiding this comment

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

These are great tests....

For safety, if anyone cared, they could be augmented by checking for cases that should not be matched...

const NON_GITHUB_URLS = [
    // a few things that are similar to URLs, and could be confused for them
];

  test('does not match string similar to URL', () => {
     NON_GITHUB_URLS(url => {
       // ......
     });
   });

@joshleaves joshleaves deleted the fix/156-linking-to-file branch January 20, 2025 09:29
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.

3 participants