-
Notifications
You must be signed in to change notification settings - Fork 232
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
feat(builder): allow environments for windows and panes #845
Conversation
Codecov Report
@@ 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
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
@zappolowski Hola! I will be able to give this a look over the weekend |
fc8d4e2
to
d59b08a
Compare
@zappolowski Rebased against 0b129c6 |
d59b08a
to
a553f99
Compare
Rebased against d63f9f5 |
a553f99
to
5ce38c9
Compare
Things that would help users see the value of the contribution:
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 |
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. |
b5a9391
to
e51af1c
Compare
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.
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 |
e51af1c
to
3a3362d
Compare
@tony I've added the feature to
Sure. |
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) |
@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 |
At 3a3362d tmux 3.2 zsh Test suitepy.test
Individual testpy.test tests/workspace/test_builder.py::test_environment_variables
|
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. |
@zappolowski Is this ready to go? Any changes you'd like to make? |
Noted! |
3a3362d
to
697b553
Compare
697b553
to
fd7ab10
Compare
fd7ab10
to
210333a
Compare
I've reworked the warnings issued for older versions of tmux to be a bit more precise and helpful and adjusted the tests accordingly. |
There was a problem hiding this 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.
@zappolowski Now live on tmuxp v1.19.0a0 (PyPI, GitHub Release, Changes on Docs, Site) Let's dogfood this! |
@zappolowski This is now live in 1.19.0! |
This fixes #832.
Open question: what's the intended/desired fallback behavior?
environment
configuration it uses theenvironment
configuration of the window. This means that in order to change one environment variable (compared to the window) the fullenvironment
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.environment
of a pane isChainMap
ped with the one of the window (and possibly the session?) which means that the finalenvironment
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).Tests are currently failing as the check for old tmux versions isn't implemented yet.