Feature/url show and repeat history #201
Conversation
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
|
WalkthroughThe changes in this pull request introduce enhancements to the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to d7e952a in 29 seconds
More details
- Looked at
203lines of code in5files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. lua/hurl/utils.lua:342
- Draft comment:
Theconvert_url_to_proper_formatfunction assumes the environment file is always specified in options. Consider adding error handling for cases where the environment file is missing or incorrectly specified. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is relevant to the changes made in the diff, as it addresses a potential issue in the newly addedconvert_url_to_proper_formatfunction. The function does log an error if the environment file cannot be opened, but it does not handle the case where the environment file is missing from the options, which the comment suggests addressing.
The function does log an error if the environment file cannot be opened, which might be considered a form of error handling. However, the comment suggests a more robust handling of the situation, which could be beneficial.
While logging an error is a form of error handling, the comment suggests a more comprehensive approach, which could improve the robustness of the function.
The comment is valid and relevant to the changes made in the diff. It suggests a potential improvement in error handling for theconvert_url_to_proper_formatfunction.
2. lua/hurl/main.lua:129
- Draft comment:
Thelast_request_optsvariable is set in multiple places (execute_hurl_cmdandrun_lines). Consider centralizing the logic for setting this variable to ensure consistency and avoid potential issues. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment points out a potential code quality issue that could lead to inconsistencies. Centralizing the logic for settinglast_request_optscould improve maintainability and reduce the risk of errors. This is a valid suggestion for a code quality refactor.
The comment does not specify how to centralize the logic, which might make it less actionable. However, it still highlights a potential issue that could be addressed.
Even though the comment does not provide a specific solution, it identifies a valid concern that could improve the code's maintainability.
The comment is valid as it highlights a potential issue with settinglast_request_optsin multiple places. It should be kept as it suggests a code quality improvement.
3. lua/hurl/main.lua:598
- Draft comment:
In theHurlReruncommand, iflast_request_optsis not set, consider notifying the user instead of just logging the message. This would improve user feedback. - Reason this comment was not posted:
Confidence changes required:50%
TheHurlReruncommand is defined to rerun the last request usinglast_request_opts. However, iflast_request_optsis not set, it logs a message but does not notify the user. Adding a notification would improve user feedback.
Workflow ID: wflow_3BHhHdS3CsVExQYy
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| @@ -312,5 +312,80 @@ util.has_file_in_opts = function(opts) | |||
|
|
|||
| return false | |||
| end | |||
| -- Function to extract the URL from the .hurl file | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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? 🙏
There was a problem hiding this comment.
@jellydn Cool! Had no time so far, I will try this weekend.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (7)
lua/hurl/init.lua (1)
8-11: LGTM! Consider adding inline documentation.The new
urlconfiguration is well-structured and aligns with the PR objectives. It provides useful defaults for showing URLs and formatting them without parameters.Consider adding inline documentation to explain the purpose of these options:
url = { -- Whether to show the URL in headers show = true, -- Whether to format the URL without query parameters format_without_params = true, },This will help users understand the purpose of these configuration options without needing to refer to external documentation.
lua/hurl/split.lua (1)
37-39: LGTM! Consider adding a safety check.The addition of the URL to the headers aligns well with the PR objectives, enhancing the functionality without disrupting existing behavior. This change will indeed make it easier for users to identify which endpoint was used for the request.
Consider adding a safety check to ensure
data.headersis a table before assigning to it. This can prevent potential errors ifdata.headersis unexpectedlynilor not a table. Here's a suggested implementation:if data.url then + if type(data.headers) ~= "table" then + data.headers = {} + end data.headers['url'] = data.url endThis change ensures that
data.headersis always a table before we attempt to add the URL to it.lua/hurl/popup.lua (1)
86-88: LGTM! Consider adding a comment for clarity.The addition of the URL to the headers when available is a good enhancement, aligning well with the PR objectives. It improves the information displayed in the popup, making it easier for users to identify the endpoint.
Consider adding a brief comment explaining the purpose of this addition for better code readability:
+ -- Include the URL in headers for better visibility in the popup if data.url then data.headers['url'] = data.url endlua/hurl/main.lua (3)
136-136: LGTM: Updates toexecute_hurl_cmdfunctionThe changes effectively implement the rerun feature and URL display functionality. The use of configuration settings for URL display provides good flexibility.
Consider adding a comment explaining the purpose of
last_request_optsfor better code readability.+-- Store the current request options for potential rerun last_request_opts = optsAlso applies to: 207-213
598-607: LGTM: Addition ofHurlReruncommandThe
HurlReruncommand is well-implemented, utilizing the stored last request options and including a necessary check.Consider adding a user notification when there's no last request to rerun for better user experience.
if last_request_opts then run_lines(last_request_opts, {}) else utils.log_info('No last request to re-run') + utils.notify('No last request to re-run', vim.log.levels.INFO) end
Line range hint
129-607: Overall implementation looks goodThe new features (rerun functionality and URL display in headers) are well-implemented and integrated into the existing codebase. They align with the PR objectives and offer good flexibility through configuration settings.
Some suggestions for further improvement:
- Consider adding more detailed comments explaining the purpose and functionality of the new features.
- It might be beneficial to add some error handling for edge cases, such as when the stored request options become invalid.
- Think about adding unit tests for the new functionalities to ensure their reliability.
lua/hurl/utils.lua (1)
343-344: Remove unnecessary commentThe comment
-- Assuming 'url' is defined earlier in the codeis unnecessary sinceurlis a parameter to the function.Apply this diff to remove the comment:
util.convert_url_to_proper_format = function(opts, url) - -- Assuming `url` is defined earlier in the code
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- lua/hurl/init.lua (1 hunks)
- lua/hurl/main.lua (4 hunks)
- lua/hurl/popup.lua (1 hunks)
- lua/hurl/split.lua (1 hunks)
- lua/hurl/utils.lua (1 hunks)
🧰 Additional context used
🔇 Additional comments (3)
lua/hurl/popup.lua (1)
Line range hint
1-188: Overall, the changes enhance functionality without disrupting existing code.The modification to include the URL in the headers is well-integrated into the existing
M.showfunction. It achieves the PR objective of providing better visibility of the endpoint while maintaining code consistency and not introducing any apparent issues.lua/hurl/main.lua (2)
129-129: LGTM: Addition oflast_request_optsvariableThe introduction of this variable is a good foundation for implementing the rerun feature. It's correctly scoped as a local variable.
312-312: LGTM: Update torun_linesfunctionThe addition of storing the last request lines using
vim.deepcopyis a good implementation. It ensures data integrity for the rerun feature when dealing with selections or specific line ranges.
lua/hurl/utils.lua
Outdated
| 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 |
There was a problem hiding this comment.
🛠️ 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.
| 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 |
lua/hurl/utils.lua
Outdated
| 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 |
There was a problem hiding this comment.
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.
| 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 |
|
Thanks @Xouzoura This is nice idea. Could you please check the review bot and fix if any issue? I will give it a try whenever you're done. |
|
@jellydn Sure, will probably do it though around end of the work-week. It should be working in general if you wanna try this out and get a feeling of it, but will need to check for the multi-line url if that's possible and what to do (can probably check and allow it only if single line for the time being or something). |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- lua/hurl/utils.lua (1 hunks)
🧰 Additional context used
🔇 Additional comments (4)
lua/hurl/utils.lua (4)
331-333: Fix potential file descriptor leakAs previously noted in a past review comment, there's a potential file descriptor leak if a match is found early in the file. The file is closed here, but not in the main loop. Consider moving the
file:close()call outside the loop to ensure it's always closed, regardless of whether a match is found.Apply this diff to fix the issue:
for line in file:lines() do -- ... (existing code) if matchcase then - file:close() return line end end + file:close()This ensures that the file is always closed after processing, preventing resource leaks.
315-391: Summary of changes and suggestionsThe new functions
get_url_from_hurl_fileandconvert_url_to_proper_formatadd valuable URL handling and environment variable substitution capabilities to the utility module. These additions align well with the PR objectives of enhancing user experience and streamlining workflows.Key points from the review:
- Improved HTTP method matching in
get_url_from_hurl_fileusing Lua's pattern matching.- Addressed potential file descriptor leak.
- Suggested optimizations for
convert_url_to_proper_format, including modularization and error handling improvements.- Recommended security enhancements for environment variable handling.
These changes, along with the suggested improvements, will significantly enhance the functionality and robustness of the hurl.nvim plugin. Please consider implementing the proposed refactors and running the provided verification scripts to ensure the changes work as expected.
343-391: Consider security implications of environment variable substitutionWhile the current implementation provides flexibility, it's important to be aware of potential security implications:
- Ensure that the .env files are not accidentally committed to version control systems.
- Be cautious about the sources of environment variables, especially if they're used in sensitive contexts (e.g., database connections).
- Consider implementing a whitelist of allowed environment variables to prevent accidental exposure of sensitive information.
To enhance security:
- Add a comment at the top of the function warning about potential security risks.
- Implement a whitelist mechanism for allowed environment variables.
Example implementation:
local ALLOWED_ENV_VARS = { 'HOST', 'PORT', 'PATH', 'SCHEME' -- Add other allowed variables here } -- ... (in the parse_env_file function) if key and value and ALLOWED_ENV_VARS[key] then env_vars[key] = value:gsub('^["\'](.-)["\']\s*$', '%1') else util.log_info('Skipping disallowed or malformed environment variable: ' .. (key or 'unknown')) endThis change ensures that only pre-approved environment variables are used in URL substitution, reducing the risk of accidentally exposing sensitive information.
To verify this security enhancement, run the following script:
#!/bin/bash # Test the URL conversion function with security enhancements # Mock the util.log_error and util.log_info functions function util.log_error() { echo "ERROR: $1" >&2 } function util.log_info() { echo "INFO: $1" >&2 } # Test cases test_cases=( "http://{{HOST}}/api/v1/users" "https://{{HOST}}:{{PORT}}/{{PATH}}" "{{SCHEME}}://{{HOST}}/{{SENSITIVE_DATA}}" ) # Mock .env file content cat << EOF > test.env HOST=example.com PORT=8080 PATH=api/v2 SCHEME=https SENSITIVE_DATA=password123 EOF # Run tests for url in "${test_cases[@]}"; do echo "Testing URL: $url" lua -e " local util = require('lua.hurl.utils') print(util.convert_url_to_proper_format({'test.env'}, '$url')) " echo done # Clean up rm test.envThis script will help verify that the security enhancements prevent the use of non-whitelisted environment variables (like SENSITIVE_DATA) in URL substitution.
343-391: 🛠️ Refactor suggestionOptimize URL conversion and improve error handling
The
convert_url_to_proper_formatfunction has a good overall structure, but there are opportunities for optimization and improved error handling:
- Consider moving the .env file search and parsing into a separate function for better modularity.
- Use
pcallwhen opening and reading the file to handle potential I/O errors gracefully.- The URL substitution could be done more efficiently using a single
gsubcall with a function.Here's a suggested refactor:
local function parse_env_file(file_path) local env_vars = {} local success, file = pcall(io.open, file_path, 'r') if not success then util.log_error('Could not open environment file: ' .. file_path) return env_vars end for line in file:lines() do local key, value = line:match('^%s*([^#%s][^=]+)%s*=%s*(.+)$') if key and value then env_vars[key] = value:gsub('^["\'](.-)["\']\s*$', '%1') end end file:close() return env_vars end util.convert_url_to_proper_format = function(opts, url) if not (url and url:find('{{')) then return url end local env_file = opts[1]:match('%.env$') and opts[1] or nil if not env_file then util.log_error('No environment file found in opts.') return url end local env_vars = parse_env_file(env_file) return url:gsub('{{(.-)}}', function(key) return env_vars[key] or '{{' .. key .. '}}' end) endThis refactored version improves error handling, reduces code duplication, and optimizes the URL substitution process.
To ensure this refactor doesn't introduce any regressions, please run the following test script:
This script will help verify that the refactored function handles various URL patterns correctly, including cases with missing environment variables.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
lua/hurl/utils.lua (1)
340-340: Remove unnecessary comment abouturlparameterThe comment
-- Assuming \url` is defined earlier in the codeis unnecessary sinceurlis a parameter of the functionutil.convert_url_to_proper_format`. Removing this comment will improve code clarity.
| 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*$", '') |
There was a problem hiding this comment.
🛠️ 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.
| value = value:gsub('^"%s*', ''):gsub('"%s*$', ''):gsub("^'%s*", ''):gsub("'%s*$", '') | |
| value = value:gsub("^%s*['\"]?", ''):gsub("['\"]?%s*$", '') |
| for key, value in pairs(env_vars) do | ||
| if url:find('{{' .. key .. '}}') then | ||
| url = url:gsub('{{' .. key .. '}}', value) | ||
| end | ||
| end |
There was a problem hiding this comment.
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.
| 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 |
| @@ -126,11 +126,15 @@ local on_output = function(code, data, event) | |||
| end | |||
| end | |||
|
|
|||
| local last_request_opts = nil | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add documentation and improve request storage logic.
The implementation of last request storage could be improved:
- Add LuaDoc comments to document the
last_request_optsvariable - Move the assignment after the running check to avoid storing failed requests
- 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 = optsAlso applies to: 135-136
| 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, | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance rerun functionality with validation and feedback.
The rerun implementation could be improved:
- Add type checking for
last_request_opts - Validate stored request data
- 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.
| 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, | |
| }) |
| 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 | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and validation for URL display.
The URL display implementation could be improved:
- Add error handling for URL extraction failures
- Validate the URL format
- 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
endCommittable suggestion was skipped due to low confidence.
|
Closed by #207 |
WHAT
Added two features that I have made that might be helpful for others.
HurlRerunWHY
Not sure if that's needed, but for me I find quite helpful to be able to rerun last request without having to go to the hurl file. Then if you make a change anywhere, you can rerun and the hurl command and see the results.
For the url, I think it makes sense to be able to see the url of the request in the endpoint headers (especially when this is not a localhost where you can see directly, but also makes easier to copy paste things. Not sure if it should go on the headers, added it there for simplicity.
So feel free to reject if you find this redundant.
HOW
url | GET {{HOST}}/user/group-accesses
or
url | GET localhost:8000/user/group-accesses
Screenshots (if appropriate):
Screenshot from 2024-10-07 11-11-34
Types of changes
Checklist:
Important
Add rerun command and URL display feature to Hurl plugin, enhancing request management and visibility.
HurlReruncommand inmain.luato rerun the last executed request using cached options.popup.luaandsplit.luaifurl.showis enabled in config.urlconfig ininit.luawith optionsshowandformat_without_params.get_url_from_hurl_file()andconvert_url_to_proper_format()inutils.luato extract and format URLs from Hurl files.This description was created by
for d7e952a. It will automatically update as commits are pushed.
Summary by CodeRabbit
Release Notes
New Features
urlconfiguration for enhanced URL handling.HurlReruncommand to easily rerun the last executed HTTP request.Improvements