fix: allow config sessions w/ duplicate paths #125
Conversation
If config sessions defined in sesh.toml have a path equal to an existing tmux session, they will not be added to a sesh List. The test added here checks whether this is the case, failing if a duplicate pathed config session is added to the session map
|
The plan was to show the test implemented in 09d5913 fail using the workflow before introducing a fix, but as running the workflow requires approval I'll submit my proposed solution below: in ...
// do not import a slice of currently listed sessions
func listConfigSessions(c *config.Config) (sessions []Session, err error) {
var configSessions []Session
// remove the now unused sessionMap
for _, sessionConfig := range c.SessionConfigs {
path := dir.AlternatePath(sessionConfig.Path)
// Do not check whether path is in session map
if sessionConfig.Name != "" {
configSessions = append(configSessions, Session{
Src: "config",
Name: sessionConfig.Name,
Path: path,
})
}
}
return configSessions, nil
}
... |
|
this fixes #124 (which is mine) |
|
I'm in the middle of a complete rewrite (#99). Do you mind waiting for that issue to be complete? I believe this is already fixed in my new branch. |
|
Sounds good. Just a couple of questions: When do you plan to release that version? Because if this is a bug and the fix is this simple, then I wouldn't mind having this implemented asap so I can start using this awesome tool! |
|
It's not really a bug IMO, just a preference. I don't have a release window yet, I've had a lot of personal things come up that have gotten in the way of me spending time working on sesh v2. You are welcome to make a quick PR if you want to change this functionality, I don't mind reviewing it and merging it before v2 ships (maybe in a month?). |
|
Okay sounds good. I'll have a go! |
From the previous commit, config sessions were displayed even though an active tmux session had been created from them, resulting in the session being displayed twice: once as a tmux session and once as a config session. This commit filters out config sessions which have an equivalent active tmux session. This is done by comparing the Path+Name combination for each config session to check whether an active tmux session fits. To be able to reliably compare paths, any paths found within the config sessions were cleaned (converted to absolute paths) on parsing the config file.
|
My attempt at a (admittedly, rather expansive) solution is ready. I ended up spending too much time on this considering you're releasing a new version so soon, but I really enjoyed working on this. You have a really cool project on your hands! With these changes, config sessions which don't have an equivalent active tmux session are always displayed with To check whether a config session has an active tmux session, the Path+Name attributes of a config session are compared with all tmux sessions. This isn't specified anywhere, but I can't see why anyone would want a config session to be separated from an active tmux session if they have the same name and are in the same location (I feel like even just the same name is enough). This was fun. I'm excited to continue using |
| // convert user home dirs to absolute paths | ||
| c.SessionConfigs[idx].Path = dir.AlternatePath(sessionConfig.Path) | ||
| c.SessionConfigs[idx].StartupScript = dir.AlternatePath(sessionConfig.StartupScript) | ||
| } |
There was a problem hiding this comment.
I noticed you're converting the sessionConfig paths to absolute paths lazily (at the moment its needed) in a couple of places.
For my Path+Name comparison of config sessions and active tmux sessions (in listConfigSessions()) I needed to convert to absolute again.
I figured that this might rather be done only once greedily (at parse time).
| Imports string | ||
| } | ||
|
|
||
| func prepareSeshConfig(t *testing.T, mockSeshConfigs []MockSeshConfig) string { |
There was a problem hiding this comment.
inject a MockSeshConfig list to enable this function to be used multiple times for many tests.
This allows the caller to create their own MockSeshConfig and get a mock config back.
| Imports string | ||
| } | ||
|
|
||
| func prepareSeshConfig(t *testing.T, mockSeshConfigs []MockSeshConfig) string { |
There was a problem hiding this comment.
this is a straight up copy from the function in config_test.go
Maybe it might be useful to put it, and the MockSeshConfig type, into a testUtils package?
|
Can you test this on sesh v2? The logic was rewritten to be more predictable, including avoiding using paths when it is a custom configuration, you do need unique session names instead. |
|
Yes sounds good. I'll try to find time this weekend to check but otherwise will test this early next week. |
|
I'll be shipping sesh v2 by the end of August, let me know if you run into any issues on your end with the new codebase. |
If config sessions defined in sesh.toml have a path equal to an existing tmux session, they are currently not added to a sesh List (when calling
sesh list.This PR adds config sessions, which are explicitly defined by the user, no independent of the user's existing tmux sessions.