Adds the file-root in the hurl cli command if there is a file or Mult… #199
Conversation
…iPart in the cached hurl file
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
|
WalkthroughThe changes introduce a new boolean variable Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 985ae22 in 18 seconds
More details
- Looked at
60lines of code in2files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. lua/hurl/utils.lua:299
- Draft comment:
Avoid usingprintfor error messages in utility functions. Consider using a logging mechanism or returning an error message instead. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_IR29LipByijcVc35
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.
lua/hurl/utils.lua
Outdated
| @@ -286,5 +286,30 @@ util.find_env_files_in_folders = function() | |||
|
|
|||
| return env_files | |||
| end | |||
| util.has_file_in_opts = function(opts) | |||
| if #opts == 0 then | |||
| print('Error: No file path provided in opts.') | |||
There was a problem hiding this comment.
Avoid using print for error messages in utility functions. Consider using a logging mechanism or returning an error message instead.
There was a problem hiding this comment.
Great. Thanks @Xouzoura Could you fix the review from AI review bot?
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
lua/hurl/main.lua (3)
195-198: Addition of--file-rootoptionThe code now adds the
--file-rootoption with the current working directory whenis_file_modeis true. This change aligns with the PR objective of enabling the use of local project files when uploading documents.However, there are a few points to consider:
- The current implementation always uses
vim.fn.getcwd()as the file root. This might not always be the desired behavior, especially if the Hurl file is in a different directory.- There's no error handling if
vim.fn.getcwd()fails for any reason.Consider the following improvements:
- Allow users to specify a custom file root:
local file_root = _HURL_GLOBAL_CONFIG.file_root or vim.fn.getcwd() vim.list_extend(cmd, { '--file-root', file_root })
- Add error handling:
local cwd = vim.fn.getcwd() if cwd then vim.list_extend(cmd, { '--file-root', cwd }) else utils.log_warn('hurl: Failed to get current working directory') end
- Consider using
vim.fn.expand('%:p:h')instead ofvim.fn.getcwd()to use the directory of the current file as the root:local file_dir = vim.fn.expand('%:p:h') vim.list_extend(cmd, { '--file-root', file_dir })
Line range hint
149-198: Overall impact of changesThe introduction of the
is_file_modecheck and the addition of the--file-rootoption enhance the functionality of the Hurl CLI command within this Lua script. These changes allow for better handling of local project files during document uploads, which aligns well with the stated PR objectives.The implementation is non-breaking and additive, maintaining compatibility with existing usage while providing new capabilities. This approach is commendable as it improves functionality without disrupting current workflows.
To further improve the robustness and flexibility of this feature:
- Consider adding a configuration option for users to specify a default file root.
- Implement error handling for edge cases, such as when the current working directory cannot be determined.
- Add documentation or comments explaining the purpose and usage of the new
is_file_modevariable and the--file-rootoption for future maintainers.
Line range hint
1-598: Suggestions for overall improvementsWhile the changes implemented are good, there are some general improvements that could be made to enhance the overall quality of the file:
Documentation: Consider adding more inline comments explaining the purpose of key functions and variables, especially for the newly added
is_file_modefunctionality.Error Handling: Implement more robust error handling throughout the file, not just for the new changes. This could include checking for nil values, handling potential exceptions, and providing informative error messages.
Configuration: Consider centralizing configuration options, including the new file root functionality, in a dedicated configuration section or file.
Testing: If not already present, add unit tests for the new functionality to ensure it works as expected and to prevent future regressions.
Refactoring: Some functions in this file are quite long and complex. Consider breaking them down into smaller, more focused functions to improve readability and maintainability.
These suggestions aim to improve the overall code quality, maintainability, and robustness of the entire file, not just the changed sections.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- lua/hurl/main.lua (2 hunks)
- lua/hurl/utils.lua (1 hunks)
🔇 Additional comments (3)
lua/hurl/utils.lua (2)
Line range hint
1-313: Request clarification on PR objectives and implementationThe new
util.has_file_in_optsfunction adds the ability to check for 'file' or 'multipart' keywords in a given file. However, its relationship to the PR objective of adding a--file-rootcommand is not immediately clear from this file alone.Could you please provide more context on how this function contributes to the implementation of the
--file-rootcommand mentioned in the PR objectives? Additionally, are there other files or changes that work in conjunction with this new function to achieve the stated goals?To gain more context, let's search for files that might be using the new
--file-rootcommand:rg --type lua "file-root"This will help us understand where and how the
--file-rootcommand is being implemented or used in the project.
289-313: 🛠️ Refactor suggestion
⚠️ Potential issueClarify the function's purpose and improve its implementation
The new
util.has_file_in_optsfunction seems to be checking for the presence of 'file' or 'multipart' keywords in a given file. However, its relationship to the PR objective of adding a--file-rootcommand is not clear. Consider the following improvements:
Rename the function to better reflect its purpose, e.g.,
util.file_contains_upload_keywords.Enhance input validation and error handling:
-util.has_file_in_opts = function(opts) - if #opts == 0 then - print('Error: No file path provided in opts.') - return false - end - - local file_path = opts[1] +util.file_contains_upload_keywords = function(file_path) + if type(file_path) ~= "string" or file_path == "" then + util.log_error('Error: Invalid file path provided.') + return false + end
- Use existing logging functions instead of
- print('Error: Failed to open file: ' .. file_path) + util.log_error('Error: Failed to open file: ' .. file_path)
- Consider adding parameters for customizable keywords and case sensitivity:
util.file_contains_keywords = function(file_path, keywords, case_sensitive) -- ... (input validation) for line in file:lines() do for _, keyword in ipairs(keywords) do if (case_sensitive and line:find(keyword)) or (not case_sensitive and line:lower():find(keyword:lower())) then file:close() return true end end end -- ... (file closure) end
- Explain how this function relates to the
--file-rootcommand mentioned in the PR objectives.To understand the context better, let's search for references to this new function:
This will help clarify how the function is being used in relation to the
--file-rootcommand.lua/hurl/main.lua (1)
149-150: New variableis_file_modeintroducedA new boolean variable
is_file_modeis added to determine if the command includes a file option. This is a good addition as it allows for conditional behavior based on whether a file is being processed.
| @@ -190,6 +192,10 @@ local function execute_hurl_cmd(opts, callback) | |||
|
|
|||
| -- Include the HTTP headers in the output and do not colorize output. | |||
| local cmd = vim.list_extend({ 'hurl', '-i', '--no-color' }, opts) | |||
| if is_file_mode then | |||
| local file_root = _HURL_GLOBAL_CONFIG.file_root or vim.fn.getcwd() | |||
There was a problem hiding this comment.
Could you add the to global variable about the new config? (file_root) @Xouzoura
|
@all-contributors add @Xouzoura for code. |
|
I've put up a pull request to add @Xouzoura! 🎉 |
Simple change to allow usage of the CWD with hurl to use endpoints that require uploading files.
WHAT
Adding the functionality to use local files of the project with hurl through the
--file-rootcommand.Example
should work now.
Link to issue
WHY
Hurl does not allow by default usage of documents if the path is not specified.
HOW
Adding current path to the hurl if there is a file uploaded (Multipart + file must exist). However modification is needed if there is a need for documents not within the project.
Screenshots (if appropriate):
Types of changes
Checklist:
Important
Adds
--file-rootoption tohurlcommand for file uploads using CWD.--file-rootwith CWD tohurlcommand if file upload is detected inexecute_hurl_cmd()inmain.lua.has_file_in_opts()inutils.luato check for file uploads in options.has_file_in_opts()inutils.luato determine if options include file uploads.execute_hurl_cmd()inmain.luato extend command with--file-rootwhen necessary.This description was created by
for 985ae22. It will automatically update as commits are pushed.
Summary by CodeRabbit
New Features
Bug Fixes