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

feat(config): allow shell parameter to be a function #423

Merged
merged 2 commits into from
Apr 9, 2023

Conversation

jakubkaczor
Copy link
Contributor

This enables determining the default shell dynamically. Please let me know if you are ok with such a change. If so, I will update the documentation.

This enables determining the default shell dynamically.
@akinsho
Copy link
Owner

akinsho commented Apr 1, 2023

@jakubkaczor thanks for the PR. Personally I don't mind at all. If you could make this query stylistically the same as other similar options that'd be great, I'm not sure the other similar options call the function inline like that. Maybe they do, on mobile so haven't checked

@jakubkaczor
Copy link
Contributor Author

@akinsho, what do you mean by “stylistically the same”? In terms of documentation?

Also, what does it mean for an option to call a function? The only function field in the configuration I located is winbar.name_formatter, which is directly called. Are you talking about the type checking of the configuration field? With this change, shell is the only field that is of a type that is a union of other types, so I don't think similarity in the field value use could be achieved.

@jakubkaczor
Copy link
Contributor Author

Excuse me. The size field can also be a function, but in the code, in the comment around declaration of the field it is documented as a number, hence my mistake. Do you mean to use something similar as ui._resolve_size function? If so, is it needed?

@akinsho
Copy link
Owner

akinsho commented Apr 4, 2023

@jakubkaczor never mind about my comment I didn't mean to cause confusion. I thought maybe this plugin declared functions in a different way I'll have a look at this again

Copy link
Owner

@akinsho akinsho left a comment

Choose a reason for hiding this comment

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

LGTM

@akinsho akinsho marked this pull request as ready for review April 9, 2023 12:46
@akinsho
Copy link
Owner

akinsho commented Apr 9, 2023

@jakubkaczor can you add documentation about this feature in the help docs and README.

Would also be nice to have a small test case that essentially launches a terminal with a function cmd and checking that it produces the correct command.

@jakubkaczor
Copy link
Contributor Author

@jakubkaczor can you add documentation about this feature in the help docs and README.

Would also be nice to have a small test case that essentially launches a terminal with a function cmd and checking that it produces the correct command.

Yes. You asked if I could add documentation to the help docs. Isn't it auto-generated by panvimdoc? Therefore, I left that out. If I shouldn't have, please clarify. 😉 And please, let me know if the test is in the form you expected.

@akinsho akinsho changed the title Allow shell parameter to be a function feat(config): allow shell parameter to be a function Apr 9, 2023
@akinsho akinsho merged commit a7857b6 into akinsho:main Apr 9, 2023
@akinsho
Copy link
Owner

akinsho commented Apr 9, 2023

@jakubkaczor changes look good thanks for making them, and the test case is fine, just wanted something in place to safeguard this feature against future changes in case myself or someone else accidentally forgets that command can be a function and it gets removed at some point

@jakubkaczor
Copy link
Contributor Author

I understand. Thanks for explanation. 😉

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