-
Notifications
You must be signed in to change notification settings - Fork 43
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
feat: refactor layout #24
Conversation
Still need to update docs |
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.
@Davincible thanks for the PR the idea looks good. This PR is quite hard to review though since it refactors the structure of the plugin, so seeing just what changes you needed to make for your addition is hard and any number of things could have broken or changed subtly and not be caught in a review.
I recommend either removing the refactoring or splitting that out into another PR first, so it can be discussed there, then follow it up later with this addition.
I'll see what I can do. Refactoring only consisted of copying some functions into other files though, so as long as they are called properly (which I think they are), everything should work |
@akinsho I've removed my color changes. Functionality should be the same as before now. Functions are pulled out into separate files. Could you have a look and merge this if you agree with it? |
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.
@Davincible tbh I think moving things into many smaller files feels a little bit like a lateral move, i.e. not sure things are more understandable now, but I don't mind the changes in general.
One thing I think should be fixed since there's no CI or autoformatting setup is that there is mixed indentation, i.e. some files are indented with tabs and others with spaces.
Can you change all the formatting to just use what it was originally, e.g. 2 spaces?
@akinsho I felt like it was easier to work with smaller files to conceptually understand what's where, felt less cluttered, which was my reasoning for separating them out. Especially in the beginning while trying to understand the code and find where I needed to be to make the relevant changes. You're right about the formatting, I've copied over the stylua.toml file from the neotest core to here, so it's all the same. |
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.
LGTM 👍🏿
3 things here)
Mandatory screenshot;
You can now define highlights like this
Currently only allows users to tweak colors, not define custom highlights. Option to add patterns could be exposed