Skip to content

feat(builder): allow environments for windows and panes #845

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

Conversation

zappolowski
Copy link
Contributor

@zappolowski zappolowski commented Nov 7, 2022

This fixes #832.

Open question: what's the intended/desired fallback behavior?

  1. (Currently implemented) If a pane doesn't have a environment configuration it uses the environment configuration of the window. This means that in order to change one environment variable (compared to the window) the full environment configuration needs to be duplicated. This also makes it possible to unset some variable if needed (compared to 2.) by skipping it int the configuration.
  2. The environment of a pane is ChainMapped with the one of the window (and possibly the session?) which means that the final environment contains all variables from both pane and window with the pane value taking precedence over the one from the window. This would avoid the possible duplication of 1. but makes it harder to unset an variable (in case an empty value doesn't suffice).
  3. Something else? Proposals welcome.

Tests are currently failing as the check for old tmux versions isn't implemented yet.

@codecov
Copy link

codecov bot commented Nov 7, 2022

Codecov Report

Merging #845 (e51af1c) into master (b7e7a79) will increase coverage by 0.13%.
The diff coverage is 100.00%.

❗ Current head e51af1c differs from pull request most recent head 210333a. Consider uploading reports for the commit 210333a to get more accurate results

@@            Coverage Diff             @@
##           master     #845      +/-   ##
==========================================
+ Coverage   76.46%   76.59%   +0.13%     
==========================================
  Files          24       24              
  Lines        1606     1615       +9     
  Branches      362      364       +2     
==========================================
+ Hits         1228     1237       +9     
  Misses        269      269              
  Partials      109      109              
Impacted Files Coverage Δ
src/tmuxp/workspace/builder.py 88.35% <100.00%> (+0.58%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@tony
Copy link
Member

tony commented Nov 12, 2022

@zappolowski Hola! I will be able to give this a look over the weekend

@tony tony force-pushed the feature/set-workspace-environment-for-windows-and-panes branch from fc8d4e2 to d59b08a Compare November 13, 2022 13:37
@tony
Copy link
Member

tony commented Nov 13, 2022

@zappolowski Rebased against 0b129c6

@tony tony force-pushed the feature/set-workspace-environment-for-windows-and-panes branch from d59b08a to a553f99 Compare November 13, 2022 14:41
@tony
Copy link
Member

tony commented Nov 13, 2022

Rebased against d63f9f5

@zappolowski zappolowski force-pushed the feature/set-workspace-environment-for-windows-and-panes branch from a553f99 to 5ce38c9 Compare November 13, 2022 15:06
@tony
Copy link
Member

tony commented Nov 13, 2022

Things that would help users see the value of the contribution:

  • An example
  • Mention the new functionality the changelog
  • Note the functionality of pane + window environments are only in versions 1.19 and beyond

I notice it's in a draft: When it's in a state where it's ready for review (that may be now) let me know

@zappolowski
Copy link
Contributor Author

It's still in draft as there's still the question what the intended behavior is (see PR message). I've added another commit (could be fixuped if needed later) to demonstrate this.

I've skipped doing the chore works until the implementation is settled.

@zappolowski zappolowski force-pushed the feature/set-workspace-environment-for-windows-and-panes branch from b5a9391 to e51af1c Compare November 13, 2022 16:06
@tony
Copy link
Member

tony commented Nov 13, 2022

@zappolowski

It's still in draft as there's still the question what the intended behavior is (see PR message). I've added another commit (could be fixuped if needed later) to demonstrate this.

  1. (Currently implemented) If a pane doesn't have a environment configuration it uses the environment configuration of the window. This means that in order to change one environment variable (compared to the window) the full environment configuration needs to be duplicated. This also makes it possible to unset some variable if needed (compared to 2.) by skipping it int the configuration.

For this PR, this is best. It's explicit, and it allows us to add support for inheriting / merging the environmental options at a later time.

  1. The environment of a pane is ChainMapped with the one of the window (and possibly the session?) which means that the final environment contains all variables from both pane and window with the pane value taking precedence over the one from the window. This would avoid the possible duplication of 1. but makes it harder to unset an variable (in case an empty value doesn't suffice).
  2. Something else? Proposals welcome.

In a version 2, I think we can introduce a system for this. I made an issue to bikeshed this here: #846

P.S. This is optional, but do you use slack? I have a channel for libtmux / tmuxp development. I can invite you

@zappolowski zappolowski force-pushed the feature/set-workspace-environment-for-windows-and-panes branch from e51af1c to 3a3362d Compare November 13, 2022 19:05
@zappolowski zappolowski marked this pull request as ready for review November 13, 2022 19:07
@zappolowski
Copy link
Contributor Author

@tony I've added the feature to CHANGES and updated the examples (also removed the last commit as this will be done later).

P.S. This is optional, but do you use slack? I have a channel for libtmux / tmuxp development. I can invite you

Sure.

@tony
Copy link
Member

tony commented Nov 13, 2022

Invited you to slack via the email in your git commits. If that doesn't work email me (via the email in the git log)

@tony
Copy link
Member

tony commented Nov 13, 2022

@zappolowski Forecast: I may not be able to get to this until next weekend (I'd ideally like to give it my undivided attention)

I am juggling a few time sensitive tasks outside tmuxp at once

@tony
Copy link
Member

tony commented Nov 19, 2022

At 3a3362d

tmux 3.2

zsh

Test suite

py.test
= FAILURES =
_____________________________________________ test_environment_variables _____________________________________________
tests/workspace/test_builder.py:364: in test_environment_variables
    assert pane.capture_pane()[1] == "PANE"
E   AssertionError: assert 't% echo $FOO' == 'PANE'
E     - PANE
E     + t% echo $FOO
        builder    = <tmuxp.workspace.builder.WorkspaceBuilder object at 0x7f169f4f3390>
        no_overrides_win = Window(@2 1:no_overrides, Session($1 libtmux_t4q8jr92))
        pane       = Pane(%4 Window(@4 3:pane_overrides, Session($1 libtmux_t4q8jr92)))
        pane_overrides_win = Window(@4 3:pane_overrides, Session($1 libtmux_t4q8jr92))
        session    = Session($1 libtmux_t4q8jr92)
        window_overrides_win = Window(@3 2:window_overrides, Session($1 libtmux_t4q8jr92))
        workspace  = {'environment': {'FOO': 'SESSION', 'PATH': '/tmp'}, 'session_name': 'test env vars', 'start_directory': '/tmp/pytest-o...es': [{'shell_command': []}, {'environment': {'FOO': 'PANE'}, 'shell_command': []}], 'window_name': 'both_overrides'}]}
= short test summary info =
FAILED tests/workspace/test_builder.py::test_environment_variables - AssertionError: assert 't% echo $FOO' == 'PANE'
= 1 failed, 199 passed, 4 skipped in 33.80s =

Individual test

py.test tests/workspace/test_builder.py::test_environment_variables
= test session starts =collected 1 item

tests/workspace/test_builder.py F                                                                                                                                                                                                     [100%]

= FAILURES =________________________________________________________________________________________________________ test_environment_variables _________________________________________________________________________________________________________tests/workspace/test_builder.py:354: in test_environment_variables
    assert pane.capture_pane()[1] == "SESSION"
E   IndexError: list index out of range
        builder    = <tmuxp.workspace.builder.WorkspaceBuilder object at 0x7f9d6fb1a750>
        no_overrides_win = Window(@2 1:no_overrides, Session($1 libtmux_4kwltjqg))
        pane       = Pane(%2 Window(@2 1:no_overrides, Session($1 libtmux_4kwltjqg)))
        session    = Session($1 libtmux_4kwltjqg)
        workspace  = {'environment': {'FOO': 'SESSION', 'PATH': '/tmp'}, 'session_name': 'test env vars', 'start_directory': '/tmp/pytest-o...es': [{'shell_command': []}, {'environment': {'FOO': 'PANE'}, 'shell_command': []}], 'window_name': 'both_overrides'}]}
= short test summary info =FAILED tests/workspace/test_builder.py::test_environment_variables - IndexError: list index out of range
= 1 failed in 0.32s ==

@tony
Copy link
Member

tony commented Nov 19, 2022

@zappolowski
Copy link
Contributor Author

@zappolowski Are you allowing commits to the PR?

Usually not (at least intentionally) as my commits are signed which won't work when other people are committing as well. So, I have to rebase anyhow to get the signatures correct.

@tony
Copy link
Member

tony commented Nov 20, 2022

@zappolowski Is this ready to go? Any changes you'd like to make?

@tony
Copy link
Member

tony commented Nov 20, 2022

Usually not (at least intentionally) as my commits are signed which won't work when other people are committing as well. So, I have to rebase anyhow to get the signatures correct.

Noted!

@zappolowski zappolowski force-pushed the feature/set-workspace-environment-for-windows-and-panes branch from 3a3362d to 697b553 Compare November 21, 2022 17:41
@zappolowski zappolowski force-pushed the feature/set-workspace-environment-for-windows-and-panes branch from 697b553 to fd7ab10 Compare November 24, 2022 17:27
@zappolowski zappolowski force-pushed the feature/set-workspace-environment-for-windows-and-panes branch from fd7ab10 to 210333a Compare November 26, 2022 17:22
@zappolowski
Copy link
Contributor Author

I've reworked the warnings issued for older versions of tmux to be a bit more precise and helpful and adjusted the tests accordingly.

Copy link
Member

@tony tony left a comment

Choose a reason for hiding this comment

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

Good!

Minor note: I may make some changes around the logging. It's a bit difficult to explain in the PR due to the nature of the change.

@tony tony merged commit 27d9552 into tmux-python:master Nov 26, 2022
tony added a commit that referenced this pull request Nov 26, 2022
@tony
Copy link
Member

tony commented Nov 26, 2022

@zappolowski Now live on tmuxp v1.19.0a0 (PyPI, GitHub Release, Changes on Docs, Site)

Let's dogfood this!

@tony
Copy link
Member

tony commented Dec 11, 2022

@zappolowski This is now live in 1.19.0!

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.

Per-window environment variables
2 participants