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

fix: handle windows file paths as uris #2640

Merged
merged 2 commits into from
Aug 10, 2023
Merged

fix: handle windows file paths as uris #2640

merged 2 commits into from
Aug 10, 2023

Conversation

TM10YMhp
Copy link
Contributor

@TM10YMhp TM10YMhp commented Aug 7, 2023

Description

Accurate detection of both URIs and Windows file paths.

path_display was not working correctly because Windows file paths were being detected as URIs

Before:
before

After:
after

Fixes #2604

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

local URI_SCHEME_PATTERN = "^([a-zA-Z]+[a-zA-Z0-9.+-]*):.*"
local WINDOWS_ROOT_PATTERN = "^[a-zA-Z]:\\"
local is_uri = function(filename)
  local is_uri_match = filename:match(URI_SCHEME_PATTERN) ~= nil
  local is_windows_root_match = filename:match(WINDOWS_ROOT_PATTERN)
  return is_uri_match and not is_windows_root_match
  --return filename:match("^%w+://") ~= nil -- before #2604
end

local uris = {
  [[https://www.example.com/index.html]],
  [[ftp://ftp.example.com/files/document.pdf]],
  [[mailto:[email protected]]],
  [[tel:+1234567890]],
  [[file:///home/user/documents/report.docx]],
  [[news:comp.lang.python]],
  [[ldap://ldap.example.com:389/dc=example,dc=com]],
  [[git://github.com/user/repo.git]],
  [[steam://run/123456]],
  [[magnet:?xt=urn:btih:6B4C3343E1C63A1BC36AEB8A3D1F52C4EDEEB096]]
}

local paths = {
  -- windows
  [[C:\Users\Usuario\Documents\archivo.txt]],
  [[D:\Projects\project_folder\source_code.py]],
  [[E:\Music\song.mp3]],
  -- macos
  [[/Users/Usuario/Documents/archivo.txt]],
  [[/Applications/App.app/Contents/MacOS/app_executable]],
  [[/Volumes/ExternalDrive/Data/file.xlsx]],
  -- linux
  [[/home/usuario/documents/archivo.txt]],
  [[/var/www/html/index.html]],
  [[/mnt/backup/backup_file.tar.gz]]
}

print('--- uris ---')
for _, uri in pairs(uris) do print(is_uri(uri)) end
print('--- paths ---')
for _, path in pairs(paths) do print(is_uri(path)) end

Configuration:

  • Neovim version (nvim --version): 0.9.1
  • Operating system and version: Windows 10

Checklist:

  • My code follows the style guidelines of this project (stylua)
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (lua annotations)

@TM10YMhp TM10YMhp marked this pull request as ready for review August 7, 2023 16:35
Copy link
Contributor

@jamestrew jamestrew left a comment

Choose a reason for hiding this comment

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

Maybe we can add that test you wrote as part of our test suite. I can do it as well.

lua/telescope/utils.lua Outdated Show resolved Hide resolved
lua/telescope/utils.lua Outdated Show resolved Hide resolved
@jamestrew
Copy link
Contributor

thanks!

@jamestrew jamestrew merged commit bc2330b into nvim-telescope:master Aug 10, 2023
6 checks passed
Comment on lines +180 to +181
local is_windows_root_match = filename:match(WINDOWS_ROOT_PATTERN)
return is_uri_match and not is_windows_root_match
Copy link
Member

Choose a reason for hiding this comment

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

we dont need to do the is_windows_root match every time, we only have to do it if is_uri_match is valid

So we can improve performance by doing return is_uri_match and not filename:match(WINDOWS_ROOT_PATTERN)

also isn't there a better is_uri_match that allows us to only match once, idk there is an rfc for this thing: https://www.rfc-editor.org/rfc/rfc3986#page-17 and we are doing the following in plenary https://github.com/nvim-lua/plenary.nvim/blob/master/lua/plenary/path.lua#L75-L77

Copy link
Contributor

@jamestrew jamestrew Aug 10, 2023

Choose a reason for hiding this comment

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

I can clean up the unnecessary is_windows_root check.

I've don't think there's a clean lua pattern matching expression that can correctly identify a URI without over matching regular paths in one go. The plenary function is not correct either. Fails for any URI without //.

Copy link
Contributor

Choose a reason for hiding this comment

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

I ran some benchmarks against roughly 80k paths (windows and linux each) for the this implementation versus short circuiting the is_windows_root check and it's not a significant difference.

Details

/tmp/testing ❯ hyperfine 'luajit current_linux.lua' 'luajit one_match_linux.lua' --warmup 10
Benchmark 1: luajit current_linux.lua
  Time (mean ± σ):      26.6 ms ±   0.8 ms    [User: 22.9 ms, System: 4.0 ms]
  Range (min … max):    25.4 ms …  30.4 ms    93 runs

Benchmark 2: luajit one_match_linux.lua
  Time (mean ± σ):      25.2 ms ±   0.8 ms    [User: 21.7 ms, System: 3.7 ms]
  Range (min … max):    24.0 ms …  27.8 ms    96 runs

Summary
  luajit one_match_linux.lua ran
    1.06 ± 0.04 times faster than luajit current_linux.lua


/tmp/testing ❯ hyperfine 'luajit current_win.lua' 'luajit one_match_win.lua' --warmup 10
Benchmark 1: luajit current_win.lua
  Time (mean ± σ):      36.7 ms ±   0.8 ms    [User: 33.5 ms, System: 3.5 ms]
  Range (min … max):    35.1 ms …  39.4 ms    69 runs

Benchmark 2: luajit one_match_win.lua
  Time (mean ± σ):      36.7 ms ±   0.8 ms    [User: 33.4 ms, System: 3.6 ms]
  Range (min … max):    35.6 ms …  39.5 ms    69 runs

Summary
  luajit one_match_win.lua ran
    1.00 ± 0.03 times faster than luajit current_win.lua

If speed is priority, we can switch to scanning the path byte by byte linearly.

Details

hyperfine 'luajit current_linux.lua' 'luajit one_match_linux.lua' 'luajit fast_linux.lua' --warmup 10
Benchmark 1: luajit current_linux.lua
  Time (mean ± σ):      24.4 ms ±   0.9 ms    [User: 20.8 ms, System: 4.0 ms]
  Range (min … max):    22.9 ms …  27.4 ms    92 runs

Benchmark 2: luajit one_match_linux.lua
  Time (mean ± σ):      22.8 ms ±   0.9 ms    [User: 19.8 ms, System: 3.4 ms]
  Range (min … max):    21.1 ms …  26.1 ms    89 runs

Benchmark 3: luajit fast_linux.lua
  Time (mean ± σ):      21.3 ms ±   0.9 ms    [User: 17.8 ms, System: 3.8 ms]
  Range (min … max):    20.0 ms …  25.6 ms    104 runs

Summary
  luajit fast_linux.lua ran
    1.07 ± 0.06 times faster than luajit one_match_linux.lua
    1.15 ± 0.06 times faster than luajit current_linux.lua


/tmp/testing ❯ hyperfine 'luajit current_win.lua' 'luajit one_match_win.lua' 'luajit fast_win.lua' --warmup 10
Benchmark 1: luajit current_win.lua
  Time (mean ± σ):      36.3 ms ±   0.9 ms    [User: 33.5 ms, System: 3.2 ms]
  Range (min … max):    34.8 ms …  40.1 ms    70 runs

Benchmark 2: luajit one_match_win.lua
  Time (mean ± σ):      36.2 ms ±   1.1 ms    [User: 34.1 ms, System: 2.6 ms]
  Range (min … max):    34.5 ms …  41.2 ms    69 runs

Benchmark 3: luajit fast_win.lua
  Time (mean ± σ):      23.3 ms ±   0.8 ms    [User: 20.3 ms, System: 3.4 ms]
  Range (min … max):    22.0 ms …  26.2 ms    97 runs

Summary
  luajit fast_win.lua ran
    1.55 ± 0.07 times faster than luajit one_match_win.lua
    1.56 ± 0.06 times faster than luajit current_win.lua

This gives minor a performance boost for linux path and ~50% boost for windows paths.

see implementation #2648

Conni2461 pushed a commit that referenced this pull request Sep 5, 2023
* fix: handle windows file paths as uris

* nit: rename FILE_PATH_PATTERN to WINDOWS_ROOT_PATTERN

(cherry picked from commit bc2330b)
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