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

Define options once at startup #183

Merged
merged 4 commits into from
Feb 18, 2021

Conversation

mvanderkamp
Copy link
Member

This also removes a couple of "getter" functions that were being used to look up options and fall back to their defaults.

I'm excited to see that this is starting to be actively maintained again! This is just a refactor suggestion so that the use of options in this plugin follows a more common idiom and such that options are easier to work with. (No need to recall the defaults every time you need to look one up).

Copy link
Member

@alerque alerque left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking the time to contribute this!

Overall I like the direction and fully agree that defaults should be handled up front and everything set rather than checking/instantiating defaults deep in the code. That being said I have a couple concerns that should probably be considered before merging this:

The big one is what looks like a regression in functionality. The previous code stored all options in such a way that they could potentially be functions instead of just hard coded values. For example g:VimuxHeight might be an integer, but it might also be a function that returns 20% of the current split. Or g:VimuxRunnerType might be just a string "pane" or it might be a function that returns different runner types based on some discovery in the current file, project, environment, etc. I'm not sure if anybody uses that, but it was there and now it's not.

Along a similar vein, it looks like everything got moved to the g: global namespace where previously in a few cases it may have been possible to get buffer-local settings to stick if they were set at the right time. This may not have been utilized either because the way I think it could have been done is pretty convoluted, but before we introduce a regression like this lets make sure we have a way to set values in an non-global way. One example where this might matter, I have projects with code in several language. I'd like to configure my runners differently depending on what language the current bufffer is.

plugin/vimux.vim Outdated Show resolved Hide resolved
plugin/vimux.vim Show resolved Hide resolved
@mvanderkamp
Copy link
Member Author

mvanderkamp commented Feb 15, 2021

Along a similar vein, it looks like everything got moved to the g: global namespace where previously in a few cases it may have been possible to get buffer-local settings to stick if they were set at the right time. This may not have been utilized either because the way I think it could have been done is pretty convoluted, but before we introduce a regression like this lets make sure we have a way to set values in an non-global way. One example where this might matter, I have projects with code in several language. I'd like to configure my runners differently depending on what language the current bufffer is.

Okay I think I've got this supported easily now, just by looking up the b: dictionary first. The neat thing here is if we also convert VimuxLastCommand and VimuxRunnerIndex to use the b: dictionary you could have buffer-local runners! My thinking for how this would work is to set both b: and g: when setting these options. So if you've set it from the current buffer you get the current buffer's runner/command, and if you didn't, you get the most recently set runner/command.

@mvanderkamp
Copy link
Member Author

Also let me know if you want this squashed / otherwise rebased down. The fixup commits are piling up...

This also removes a couple of "getter" functions that were being used to
look up options and fall back to their defaults
Copy link
Member

@alerque alerque left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I rebased this for you to get some style fixes I made on master and keep the indentation and whitespace consistent in the PR. I didn't change anything substantial other than style stuff.

plugin/vimux.vim Show resolved Hide resolved
plugin/vimux.vim Show resolved Hide resolved
plugin/vimux.vim Outdated Show resolved Hide resolved
@alerque alerque merged commit 8cc4fe9 into preservim:master Feb 18, 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.

2 participants