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(api): customizable at_edge behavior #80

Merged
merged 8 commits into from
Apr 17, 2023

Conversation

mrjones2014
Copy link
Owner

@mrjones2014 mrjones2014 commented Apr 14, 2023

Changes config.wrap_at_edge = boolean to config.at_edge = 'wrap'|'split'|'stop'.

Fixes #79

@mrjones2014
Copy link
Owner Author

@IndianBoy42 give this a test please

@IndianBoy42
Copy link

at_edge="split" certainly works how I expected, thanks!

I dont use any mux so I can't check that,

@mrjones2014
Copy link
Owner Author

Cool, I'm going to use this branch for a while to make sure I'm not introducing any regressions before merging.

README.md Show resolved Hide resolved
@mrjones2014
Copy link
Owner Author

mrjones2014 commented Apr 14, 2023

@mehalter I'd like to get your thoughts on whether you think these types of changes should be considered breaking changes, and therefore require a Major release.

Technically nothing is "broken," as the changes in this PR will migrate configuration settings to their new equivalents, however, you'll see a deprecation notice if you've explicitly set any of the affected settings in config.

For example, if you have either of these explicitly in config:

require('smart-splits').setup({ wrap_at_edge = true })
-- or
require('smart-splits').setup({ wrap_at_edge = false })

Then the plugin will migrate this to the new config.at_edge = 'wrap' or config.at_edge = 'stop', respectively, and you'll get a deprecation notice that config.wrap_at_edge is deprecated, and use to use config.at_edge = 'wrap'|'split'|'stop' instead.

So I guess the tl;dr is: Should it be considered a "breaking change" if nothing is technically "broken," but explicitly setting the deprecated configs will print a deprecation notice using vim.deprecate()?

I'm bringing this up given the context that I recently learned this plugin is used in AstroNvim, and I want to know how you'd like to see the versioning scheme fit here.

@mrjones2014
Copy link
Owner Author

Going to merge, but would still like some feedback about next release tag from @mehalter.

@mrjones2014 mrjones2014 merged commit 4b7dd22 into master Apr 17, 2023
@mrjones2014 mrjones2014 deleted the mrj/79/smart-new-splits branch April 17, 2023 10:47
@mehalter
Copy link

@mrjones2014 I think tagging those options as deprecated and leaving some backwards compatibility for the true/false notation is definitely alright to avoid a breaking version right now! I will update AstroNvim's core code after you tag the next release (non-breaking) and then people who are using true/false as an override (which I am imagining are not many people) will be able to update their code.

@mehalter
Copy link

also sorry about the late reply, was out of town for the past few days and totally forgot to go back and answer this! Thanks so much for the heads up :) Love the community and joint collaboration ❤️

@mehalter
Copy link

Also just realized that AstroNvim doesn't actually override this setting, so the base astronvim doesn't require a code update, and it will just affect users who override this explicitly which there is a certain level of responsibility taken when customizing AstroNvim 😄

@mrjones2014
Copy link
Owner Author

also sorry about the late reply

No worries!

Thanks for that feedback, I will tag a minor release shortly then! 💪

@mehalter
Copy link

@mrjones2014 just while we are chatting here and to avoid making an issue. Do you think it would make sense to have at_edge = "split" respect ignored_buftypes? Basically if I am in an ignored buftype and move in a direction that would create a split I think it would make sense to ignore it. It gets a bit weird if i'm in my tree for example on the left hand side (like a sidebar), moving left on accident, and getting a second tree split also on the side. Let me know what you think :)

@mrjones2014
Copy link
Owner Author

Hmm, generally yeah that sounds like a good idea, but it might get a bit weird in certain edge cases, but maybe we can work around those.

What happens if your edge is an ignored window, and you have a mux integration enabled, but no mux window next to the ignored window? Possibly we could just ignore it and basically do at_edge = 'stop' or at_edge = 'wrap' in this case, but how do we decide which?

@mehalter
Copy link

I guess I was thinking split implies that you aren't wrapping. So I would think it would just do stop

@mrjones2014
Copy link
Owner Author

Sounds fair enough. #83

@mehalter
Copy link

Thanks so much!

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.

[Feature]: Smart split creation
3 participants