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 #30 – user plugins take priority over built-in plugins #73

Merged
merged 4 commits into from Mar 3, 2021

Conversation

liquidev
Copy link
Collaborator

@liquidev liquidev commented Mar 2, 2021

I don't know how ugly this solution is, considering that it messes with the package search path. In my opinion the plugin system should be changed not to use require though, and use dofile() together with setting keys in package.loaded or package.preload, so that it's not prone to cases like this. On the other hand, some plugins like my own lint+ are composed of multiple modules, so having the package search path contain the user configuration directory is quite useful. I'm just not sure about this change, which involves changing the order of package search paths in package.path.

Closes #30

@liquidev
Copy link
Collaborator Author

liquidev commented Mar 2, 2021

GitHub didn't pick up on the title, but this closes #30

Edit: This needs to be in the main message, so ignore this comment.

@liquidev liquidev requested a review from franko March 2, 2021 17:20
data/core/init.lua Outdated Show resolved Hide resolved
data/core/init.lua Outdated Show resolved Hide resolved
@franko
Copy link
Member

franko commented Mar 2, 2021

Thank you, this is nicely done. I just made a couple of comments, please clarify. Otherwise I am not sure how the github's "review" thing works.

@djmattyg007
Copy link

@franko You could look over Github's own documentation on the subject :)

https://docs.github.com/en/github/collaborating-with-issues-and-pull-requests/about-pull-request-reviews

@liquidev liquidev requested a review from franko March 3, 2021 11:15
@franko franko merged commit 2e1a6ad into master Mar 3, 2021
@franko
Copy link
Member

franko commented Mar 3, 2021

Thank you @liquidev

@franko franko deleted the lqdev-plugin-priority branch March 3, 2021 15:54
@franko
Copy link
Member

franko commented Mar 3, 2021

Hey @liquidev I think we have been too hasty with this patch, it just does not work! I get this:

Error: /home/francesco/dev/lite/.run/share/lite-xl/core/init.lua:542: attempt to index a nil value
stack traceback:
        /home/francesco/dev/lite/.run/share/lite-xl/core/init.lua:542: in function 'load_plugins'
        /home/francesco/dev/lite/.run/share/lite-xl/core/init.lua:430: in function 'init'
        [string "local core..."]:7: in function <[string "local core..."]:2>
        [C]: in function 'xpcall'
        [string "local core..."]:2: in main chunk

I think the problem is that you are using the file information but the value can be nil. By looking more closely I didn't have time to give a solution but it seems that the logic should be tightened. May be you can go back to the original implementation and only apply minimal modifications by strictly verifying that each modification is sound.

I advice to not use common.basename and stick to the old implementation, just make the modification to ensure the files ends with the ".lua" extension.

@liquidev
Copy link
Collaborator Author

liquidev commented Mar 3, 2021

@franko I'll work on that tomorrow, I turned my computer off for the night already. In the meantime, could you revert the changes I made?

@franko
Copy link
Member

franko commented Mar 3, 2021

Don't worry, there is no need to revert, nothing bad will happen, you can fix this later when you want.

franko added a commit that referenced this pull request Mar 5, 2021
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.

Override a pre-installed plugin
3 participants