-
Notifications
You must be signed in to change notification settings - Fork 28
Model related files picker #45
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
Model related files picker #45
Conversation
|
|
||
| local class = get_model_class_name() | ||
| if class ~= "" then | ||
| local result, ok = application.run("artisan", { "model:show", class, "--json" }, { runner = "sync" }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The command could halt if doctrine DBAL is not install, we need to add a check for it before invoking the command.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you encounter this issue during your tests? Which Laravel version and which DB did you use, as this is honestly a bit weird. To my knowledge model:show command does not use such methods that would require DBAL, but i may be mistaken.
By halting do you mean not outputting errors or outputting raw error message or do you really mean that it goes into infinite loop?
| :find() | ||
| end | ||
|
|
||
| local related = function(opts) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file is getting really big. Please move this function into a new file lets use lua/laravel/telescope/pickers/related.lua and require for the return. I will move the others.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure
|
|
||
| local model_info = vim.fn.json_decode(result.out[1]) | ||
| if model_info == nil then | ||
| return nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should notify that could not be parse, if not the user experience will be bad.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, will improve error messages for sure
| end | ||
|
|
||
| if result.exit_code ~= 0 or string.sub(result.out[1], 1, 1) ~= "{" or string.sub(result.out[1], -1) ~= "}" then | ||
| return nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Notify the user, and maybe log the error.
Since there is no validation of the class that is a model it can fail easily.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea is that we don't really need to check for the model parent class, because the artisan command does it for us, as it expects the provided class to have some methods. We can notify the user though that command errors probably due to the class not being the Model.
We can add the check, I am just not sure we really need it. For now I cannot see another reason for the command to fail if artisan is runnable, there may be, but I think we can handle it once we see those issues. We could display the notification: "the \App\SomeModel is not a model" or something along these lines, if for some reason the tree sitter query fails, the user will clearly see that in the notification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The luacheck is failing due wrong indent. please fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will do, it will take some time as there are some personal things I am going through and work has piled up as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adalessa i am back and will do the changes soon
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adalessa fixes are pushed, please check.
This is an initial version that utilizes
model:showartisan command. In future may be improved to also include files with similar names, but i think for now we should try this and see whee it takes us.closes #14