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

Feature/url show and repeat history #201

Closed
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions lua/hurl/init.lua
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,10 @@ local default_config = {
mode = 'split',
show_notification = false,
auto_close = true,
url = {
show = true,
format_without_params = true,
},
-- Default split options
split_position = 'right',
split_size = '50%',
Expand Down
24 changes: 24 additions & 0 deletions lua/hurl/main.lua
Original file line number Diff line number Diff line change
Expand Up @@ -126,11 +126,15 @@ local on_output = function(code, data, event)
end
end

local last_request_opts = nil
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add documentation and improve request storage logic.

The implementation of last request storage could be improved:

  1. Add LuaDoc comments to document the last_request_opts variable
  2. Move the assignment after the running check to avoid storing failed requests
  3. Consider adding a cleanup mechanism for the stored request
--- Variable declaration
+--- @type table|nil
+--- @field opts table The options for the last successful request
 local last_request_opts = nil

 local function execute_hurl_cmd(opts, callback)
   if is_running then
     utils.log_info('hurl: request is already running')
     utils.notify('hurl: request is running. Please try again later.', vim.log.levels.INFO)
     return
   end
+  last_request_opts = opts
-  last_request_opts = opts

Also applies to: 135-136

--- Call hurl command
---@param opts table The options
---@param callback? function The callback function
local function execute_hurl_cmd(opts, callback)
-- Check if a request is currently running

last_request_opts = opts

if is_running then
utils.log_info('hurl: request is already running')
utils.notify('hurl: request is running. Please try again later.', vim.log.levels.INFO)
Expand Down Expand Up @@ -200,6 +204,14 @@ local function execute_hurl_cmd(opts, callback)

utils.log_info('hurl: running command' .. vim.inspect(cmd))

if _HURL_GLOBAL_CONFIG.url.show then
local url = utils.get_url_from_hurl_file(opts[1])
if _HURL_GLOBAL_CONFIG.url.format_without_params then
url = utils.convert_url_to_proper_format(opts, url)
end
response.url = url
end

Comment on lines +207 to +214
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add error handling and validation for URL display.

The URL display implementation could be improved:

  1. Add error handling for URL extraction failures
  2. Validate the URL format
  3. Document the URL configuration options
 if _HURL_GLOBAL_CONFIG.url.show then
-  local url = utils.get_url_from_hurl_file(opts[1])
+  local url, err = utils.get_url_from_hurl_file(opts[1])
+  if err then
+    utils.log_warn('hurl: failed to extract URL: ' .. err)
+    return
+  end
+  if not url:match('^https?://') then
+    utils.log_warn('hurl: invalid URL format: ' .. url)
+    return
+  end
   if _HURL_GLOBAL_CONFIG.url.format_without_params then
     url = utils.convert_url_to_proper_format(opts, url)
   end
   response.url = url
 end

Committable suggestion was skipped due to low confidence.

vim.fn.jobstart(cmd, {
on_stdout = callback or (is_json_mode and on_json_output or on_output),
on_stderr = callback or (is_json_mode and on_json_output or on_output),
Expand Down Expand Up @@ -297,6 +309,7 @@ local function run_lines(lines, opts, callback)
-- Clean up the temporary file after a delay
local timeout = 1000
vim.defer_fn(function()
last_request_opts = vim.deepcopy(lines)
local success = os.remove(fname)
if not success then
utils.log_info('hurl: remove file failed ' .. fname)
Expand Down Expand Up @@ -581,6 +594,17 @@ function M.setup()
nargs = '*',
range = true,
})

utils.create_cmd('HurlRerun', function()
if last_request_opts then
run_lines(last_request_opts, {})
else
utils.log_info('No last request to re-run')
end
end, {
nargs = '*',
range = true,
})
Comment on lines +598 to +607
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance rerun functionality with validation and feedback.

The rerun implementation could be improved:

  1. Add type checking for last_request_opts
  2. Validate stored request data
  3. Add user feedback for successful rerun
 utils.create_cmd('HurlRerun', function()
   if last_request_opts then
+    if type(last_request_opts) ~= 'table' then
+      utils.notify('Invalid last request data', vim.log.levels.ERROR)
+      return
+    end
+    if vim.tbl_isempty(last_request_opts) then
+      utils.notify('Empty last request data', vim.log.levels.ERROR)
+      return
+    end
     run_lines(last_request_opts, {})
+    utils.notify('Rerunning last request...', vim.log.levels.INFO)
   else
-    utils.log_info('No last request to re-run')
+    utils.notify('No last request to re-run', vim.log.levels.WARN)
   end
 end, {
   nargs = '*',
   range = true,
 })
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
utils.create_cmd('HurlRerun', function()
if last_request_opts then
run_lines(last_request_opts, {})
else
utils.log_info('No last request to re-run')
end
end, {
nargs = '*',
range = true,
})
utils.create_cmd('HurlRerun', function()
if last_request_opts then
if type(last_request_opts) ~= 'table' then
utils.notify('Invalid last request data', vim.log.levels.ERROR)
return
end
if vim.tbl_isempty(last_request_opts) then
utils.notify('Empty last request data', vim.log.levels.ERROR)
return
end
run_lines(last_request_opts, {})
utils.notify('Rerunning last request...', vim.log.levels.INFO)
else
utils.notify('No last request to re-run', vim.log.levels.WARN)
end
end, {
nargs = '*',
range = true,
})

end

return M
5 changes: 3 additions & 2 deletions lua/hurl/popup.lua
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,9 @@ M.show = function(data, type)
popups.bottom:map('n', _HURL_GLOBAL_CONFIG.mappings.prev_panel, function()
vim.api.nvim_set_current_win(popups.top.winid)
end)

if data.url then
data.headers['url'] = data.url
end
-- Add headers to the top
local headers_table = utils.render_header_table(data.headers)
-- Hide header block if empty headers
Expand All @@ -95,7 +97,6 @@ M.show = function(data, type)
end
end

-- Add response time as virtual text
vim.api.nvim_buf_set_extmark(
popups.top.bufnr,
vim.api.nvim_create_namespace('response_time_ns'),
Expand Down
4 changes: 3 additions & 1 deletion lua/hurl/split.lua
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,9 @@ M.show = function(data, type)
quit()
end)
end

if data.url then
data.headers['url'] = data.url
end
-- Add headers to the top
local headers_table = utils.render_header_table(data.headers)
-- Hide header block if empty headers
Expand Down
75 changes: 75 additions & 0 deletions lua/hurl/utils.lua
Original file line number Diff line number Diff line change
Expand Up @@ -312,5 +312,80 @@ util.has_file_in_opts = function(opts)

return false
end
-- Function to extract the URL from the .hurl file
Copy link

Choose a reason for hiding this comment

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

The get_url_from_hurl_file function may not accurately extract URLs if the line contains more than just the URL or if the URL is split across lines. Consider improving the logic to extract only the URL part.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If as a concept the PR is liked and this is an issue, I can improve that. It works for single line urls currently, but it is right that it might have an issue if you split (if somebody splits the url ofc).

Let me know

Copy link
Owner

Choose a reason for hiding this comment

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

Hi @Xouzoura Thank you for your PR. Yeah, I like your idea on this. Could you please check and fix the review bots when you're available? 🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jellydn Cool! Had no time so far, I will try this weekend.

util.get_url_from_hurl_file = function(file_path)
local url = nil
local file = io.open(file_path, 'r')

if file then
for line in file:lines() do
line = line:gsub('^%s*(.-)%s*$', '%1')
line = line:gsub('%s+', ' ')
-- NOTE: somehow i can not make regex work here
local matchcase = string.find(line, 'GET ')
or string.find(line, 'POST ')
or string.find(line, 'PUT ')
or string.find(line, 'DELETE ')
or string.find(line, 'PATCH ')
if matchcase then
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Simplify HTTP method matching logic

Consider using pattern matching or a table to simplify the HTTP method detection, improving readability and maintainability.

Apply this diff to refactor the code:

         line = line:gsub('%s+', ' ')
-        -- NOTE: somehow i can not make regex work here
-        local matchcase = string.find(line, 'GET ')
-          or string.find(line, 'POST ')
-          or string.find(line, 'PUT ')
-          or string.find(line, 'DELETE ')
-          or string.find(line, 'PATCH ')
-        if matchcase then
+        local method = line:match('^(%w+)%s')
+        local http_methods = { GET = true, POST = true, PUT = true, DELETE = true, PATCH = true }
+        if http_methods[method] then
           file:close()
           return line
         end
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
local matchcase = string.find(line, 'GET ')
or string.find(line, 'POST ')
or string.find(line, 'PUT ')
or string.find(line, 'DELETE ')
or string.find(line, 'PATCH ')
if matchcase then
line = line:gsub('%s+', ' ')
local method = line:match('^(%w+)%s')
local http_methods = { GET = true, POST = true, PUT = true, DELETE = true, PATCH = true }
if http_methods[method] then

jellydn marked this conversation as resolved.
Show resolved Hide resolved
return line
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix potential file descriptor leak on early return

In util.get_url_from_hurl_file, if a match is found, the function returns immediately without closing the file, which could lead to a file descriptor leak.

Apply this diff to ensure the file is closed properly:

 for line in file:lines() do
   line = line:gsub('^%s*(.-)%s*$', '%1')
   line = line:gsub('%s+', ' ')
   -- NOTE: somehow i can not make regex work here
   local matchcase = string.find(line, 'GET ')
     or string.find(line, 'POST ')
     or string.find(line, 'PUT ')
     or string.find(line, 'DELETE ')
     or string.find(line, 'PATCH ')
   if matchcase then
+    file:close()
     return line
   end
 end
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if file then
for line in file:lines() do
line = line:gsub('^%s*(.-)%s*$', '%1')
line = line:gsub('%s+', ' ')
-- NOTE: somehow i can not make regex work here
local matchcase = string.find(line, 'GET ')
or string.find(line, 'POST ')
or string.find(line, 'PUT ')
or string.find(line, 'DELETE ')
or string.find(line, 'PATCH ')
if matchcase then
return line
if file then
for line in file:lines() do
line = line:gsub('^%s*(.-)%s*$', '%1')
line = line:gsub('%s+', ' ')
-- NOTE: somehow i can not make regex work here
local matchcase = string.find(line, 'GET ')
or string.find(line, 'POST ')
or string.find(line, 'PUT ')
or string.find(line, 'DELETE ')
or string.find(line, 'PATCH ')
if matchcase then
file:close()
return line

end
end
file:close()
else
util.log_info('Could not open file: ' .. file_path)
end

return url
end

util.convert_url_to_proper_format = function(opts, url)
-- Assuming `url` is defined earlier in the code
if url and url:find('{{') then -- Check if url contains '{{'
local env_file

-- Find the environment file in the opts that ends with .env
for _, opt in ipairs(opts) do
if opt:match('%.env$') then -- Check if the option ends with .env
env_file = opt
break -- Exit the loop once the first .env file is found
end
end
if env_file then
-- Read the environment file and get the variables
local env_vars = {}
local file = io.open(env_file, 'r')

if not file then
util.log_error('Could not open environment file: ' .. env_file)
else
-- Read each line of the file
for line in file:lines() do
-- Skip empty lines and comments
line = line:match('^%s*(.-)%s*$') -- Trim whitespace
if line ~= '' and not line:match('^#') then
local key, value = line:match('^(%S+)%s*=%s*(.+)$') -- Match key=value
if key and value then
-- Trim quotes from value, if present
value = value:gsub('^"%s*', ''):gsub('"%s*$', ''):gsub("^'%s*", ''):gsub("'%s*$", '')
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Simplify and correct the quote trimming logic

The current code for trimming quotes from the value variable can be simplified and made more robust. Consider using patterns that remove both single and double quotes from the start and end of the string, including any surrounding whitespace.

Apply this diff to improve the quote trimming:

- value = value:gsub('^"%s*', ''):gsub('"%s*$', ''):gsub("^'%s*", ''):gsub("'%s*$", '')
+ value = value:gsub("^%s*['\"]?", ''):gsub("['\"]?%s*$", '')
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
value = value:gsub('^"%s*', ''):gsub('"%s*$', ''):gsub("^'%s*", ''):gsub("'%s*$", '')
value = value:gsub("^%s*['\"]?", ''):gsub("['\"]?%s*$", '')

env_vars[key] = value
end
end
end
file:close()
end

for key, value in pairs(env_vars) do
if url:find('{{' .. key .. '}}') then
url = url:gsub('{{' .. key .. '}}', value)
end
end
Comment on lines +374 to +378
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Handle unresolved placeholders in URL

If the URL contains placeholders that are not found in the environment variables, the placeholders will remain in the URL, which may cause issues in downstream processing. Consider adding a check for unresolved placeholders and logging a warning if any are found.

Apply this diff to handle unresolved placeholders:

       end
     end

+    -- After replacements, check for unresolved placeholders
+    if url:find('{{.-}}') then
+      util.log_error('Unresolved placeholders remain in URL: ' .. url)
+    end
   else
     util.log_error('No environment file found in opts.')
   end
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
for key, value in pairs(env_vars) do
if url:find('{{' .. key .. '}}') then
url = url:gsub('{{' .. key .. '}}', value)
end
end
for key, value in pairs(env_vars) do
if url:find('{{' .. key .. '}}') then
url = url:gsub('{{' .. key .. '}}', value)
end
end
-- After replacements, check for unresolved placeholders
if url:find('{{.-}}') then
util.log_error('Unresolved placeholders remain in URL: ' .. url)
end

else
util.log_error('No environment file found in opts.')
end
-- Load environment variables from the found env_file
end

return url
end
return util
Loading