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.

Sign up for GitHub

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

Jump to bottom

Add several updates to vim._with (tests, granular option contexts, env context) #29280

Merged
merged 3 commits into from
Jun 26, 2024
Merged

Add several updates to vim._with (tests, granular option contexts, env context) #29280

merged 3 commits into from
Jun 26, 2024
+1,658 −243

Conversation

echasnovski
Copy link
Member

@echasnovski echasnovski commented Jun 11, 2024 •
edited
Loading

This PR evolved into three improvements:

  • Massively cover existing functionality with tests. Some (many) tests are marked as pending because the particular context does not work. Those tests are meant as a reference for future fixing.
  • Replace options context with o, bo, wo, go granular contexts. This aligns with existing Lua options terminology and is more flexible than single options.
  • Add env context for environment variables. As this also aligns with vim.env.

More details in commits' messages.

Resolves #29253

Alternative to #29269 and #29268

@dundargoc dundargoc marked this pull request as draft June 11, 2024 09:10
This was referenced Jun 11, 2024
Closed
Closed
Open
@github-actions github-actions bot added the lua stdlib label Jun 11, 2024
@echasnovski echasnovski force-pushed the with-owobogo branch 4 times, most recently from 37d65a6 to 8b208ff Compare June 19, 2024 09:11
@echasnovski echasnovski force-pushed the with-owobogo branch 6 times, most recently from 77bcbfb to cde6619 Compare June 21, 2024 13:02
@echasnovski echasnovski changed the title feat(lua): update vim._with to allow more granular context Add several updates to vim._with (tests, granular option contexts, env context) Jun 21, 2024
@echasnovski echasnovski marked this pull request as ready for review June 21, 2024 13:20
dundargoc
dundargoc approved these changes Jun 22, 2024
View reviewed changes
Copy link
Member

@dundargoc dundargoc left a comment

Choose a reason for hiding this comment

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

I haven't checked the test code but I assume they hold the standard echa quality. The option code is somewhat hard to follow, but I'm unsure if there's a good way to simplify it tbh. Maybe we can look into that later. All in all looks great. All the test cases are super valuable.

@echasnovski echasnovski force-pushed the with-owobogo branch 2 times, most recently from 1a3e025 to 868771c Compare June 23, 2024 14:14
@echasnovski
test(lua): cover vim._with() with tests
f879536 
Problem: `vim._with()` has many different use cases which are not
  covered with tests.

Solution: cover with tests. Some (many) test cases are intentionally
  marked as "pending" because they cover cases which don't work as
  expected at the moment (and fixing them requires specific knowledge of
  C codebase). Use them as a reference for future fixes.
  Also some of "can be nested" tests currently might pass only because
  the tested context doesn't work.
@echasnovski
feat(lua): update vim._with to allow more granular option contexts
07cc559 
Problem: with a single `context.options` there is no way for user to
  force which scope (local, global, both) is being temporarily set and
  later restored.

Solution: replace single `options` context with `bo`, `go`, `wo`, and
  `o`. Naming and implementation follows how options can be set directly
  with `vim.*` (like `vim.bo`, etc.).
  Options are set for possible target `win` or `buf` context.
@echasnovski
feat(lua): add context.env (environment variables) to vim._with()
cd53db2
@dundargoc dundargoc merged commit 76dd07e into neovim:master Jun 26, 2024
29 checks passed
@github-actions github-actions bot removed the request for review from gpanders June 26, 2024 10:23
@dundargoc
Copy link
Member

This is super valuable. Thanks for championing this.

Follow-up work (not necessarily by you, I'm talking in general): ensure all the tests marked as pending passes. It looks like it involved primarily ensuring that vim._with can be nested which will need adjustments in the C code.

@echasnovski
Copy link
Member Author

Follow-up work (not necessarily by you, I'm talking in general): ensure all the tests marked as pending passes. It looks like it involved primarily ensuring that vim._with can be nested which will need adjustments in the C code.

Not only those, I am afraid. Quite a few context fields coming from command modifiers don't seem to work:

Soo... basically most part of the flags :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gpanders gpanders gpanders left review comments

@dundargoc dundargoc dundargoc approved these changes

@justinmk justinmk justinmk approved these changes

Assignees
No one assigned
Labels
ci:skip-news lua stdlib
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

vim._with() does not properly restore global-local options
4 participants
@echasnovski @dundargoc @justinmk @gpanders