- 
                Notifications
    You must be signed in to change notification settings 
- Fork 233
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.testIndividual 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?
environmentconfiguration it uses theenvironmentconfiguration of the window. This means that in order to change one environment variable (compared to the window) the fullenvironmentconfiguration needs to be duplicated. This also makes it possible to unset some variable if needed (compared to 2.) by skipping it int the configuration.environmentof a pane isChainMapped with the one of the window (and possibly the session?) which means that the finalenvironmentcontains 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.