Adds support for tmuxinator start --append #652

flovilmart · 2018-09-16T18:43:36Z

Closes: #265

ethagnawl · 2018-09-19T03:45:59Z

Thanks for this, @flovilmart! I'll try to give it a proper review before the end of the week.

flovilmart · 2018-09-19T04:24:41Z

Take your time @ethagnawl! No rush for that, i'm using it at the moment and probably will weed out some odd edge cases. Also, if you need I can try to add some tests. I'm not very used to ruby though, so it could take time.

DestyNova · 2018-09-19T21:36:52Z

Very nice. The only confusing bit is that it names the new window <session-name>:<window-name> -- i.e. if I append a window called muffin from a file containing the line name: mountain, then the new window will be called mountain:muffin. If I don't specify the name parameter in the Yaml file, then the window will be called :muffin.

I see there is some logic to control this behaviour, but I don't fully understand it. Is there an extra flag or configuration value needed to just have the window be called muffin?

flovilmart · 2018-09-19T22:01:36Z

@DestyNova that would be a possible change, if the name is not set with the tmux template I could probably remove the separator. This was intended to make it easy to see which windows were appended.

DestyNova · 2018-09-19T22:18:15Z

Yes I see the motivation for it and it makes sense -- in my case though, I expect to end up with a bunch of files that each describe one window with a couple of panes. Since I use windows and panes where other people might use sessions and windows, I don't need that namespacing. Maybe the behaviour could be specified with an optional flag (e.g. if a simple name is default, then add -p for prefix-with-session-name, or if the namespaced window name is default, then -s for simple-window-name or something)?

flovilmart · 2018-09-19T22:33:33Z

I’m not sure I want to spend additional time adding more configuration flags to this PR. If the naming of the windows is problematic i’ll Remove it altogether and restore the original behavior instead of prefixing with the screen name.

DestyNova · 2018-09-19T22:37:41Z

I like the PR either way and am really happy someone has finally done this. Rather than sidetrack the PR, I'll try to do a separate one adding such a flag afterwards. Thanks for doing this :)

ethagnawl · 2018-10-30T04:48:51Z

@flovilmart This is looking pretty good!

I do have a few questions/thoughts/requests/etc. for you:

1.) Would you mind adding an entry to the "Other Commands" section of README.md in your PR which documents how this feature and its various CLI options are intended to be used?

The documentation will be useful for users going forward and for people reviewing this PR. I wasn't following #265 closely and it took me a few minutes to figure out how this feature was intended to work.

2.) A missing/malformed current_session: ... seems to result in tmuxinator wantonly appending windows to whatever the most recently created tmux session is, which seems undesirable. Is this by design? Is there a way you can sniff out whether or not the current_session exists and either fail or start a new session if it doesn't? (I'll revisit the code myself, but I'm on my way out for the day.)

3.) Could you replace puts with say in Cli.rb?

4.) Perhaps the "Creating new session ..." message should contain the session name?

5.) We will need some tests around this new behavior. Did you ever get a chance to take a pass at writing any? I know you said you're not used to Ruby, so I can appreciate how that might be a bit daunting and it's awesome that you were able to put this together. (I don't think you're giving yourself enough credit!) If not, that's totally fine and I can take a pass at writing some and submit another PR to your branch.

flovilmart · 2018-10-30T11:25:29Z

All of those make sense, i’ll Have a look at the requests and will do the tests as well.

I’m only an amateur rubyist, punching letters in to get the expected result as such as often fail to recognize idiomatic patterns. Therefore the code I produce is most likely to be linearly dumb :)
Yet those are no excuses for not trying :)

flovilmart · 2018-11-10T15:07:52Z

I Didn’t forget about this one, but my todo list keeps pushing it down. I hope to have more time soon

raiyuza · 2019-02-08T13:02:03Z

@flovilmart Any ETA, we in 2019 already :)

flovilmart · 2019-02-08T13:34:38Z

Well, yea, I need to but my head back into it :)

_{Sent with GitHawk}

icole · 2019-04-05T18:27:42Z

Any issue with getting this merged now that the merge conflicts are resolved?

flovilmart · 2019-04-05T18:53:55Z

@icole I need to work on the few tests and documentation. I haven't found a good time to do it. I'll try this weekend.

icole · 2019-04-05T19:06:56Z

@flovilmart awesome, thanks for the response. If you could use any help with anything let me know. Would love to see this go out.

flovilmart · 2019-04-05T19:17:44Z

Thanks @icole I'll do my best to address all the concerns and add the relevant tests. I'd love to get done with this one. Sorry to have dropped the ball.

flovilmart · 2019-06-26T21:19:53Z

@icole Sorry for the late response. I changed a bit the logic so it uses mux start --append [project] instead of mux append. This simplifies a bit the logic.

I'll add some tests around the in_current_session logic to make sure this is readable easily and maintainable as well.

icole · 2019-06-28T23:25:11Z

@flovilmart thanks so much! could we pull this in after you add those tests? I am dying to get this in since the version this is based off is older than what our projects Gemfile requires.

betoharres · 2020-02-15T02:05:46Z

I was trying to figure out if tmuxinator had this feature; well, hope it gets merged!

flovilmart · 2020-02-15T12:12:45Z

I completely forgot I had this one open, thanks for the ping 😅! I’ll do my best.

protex · 2020-07-22T13:10:54Z

Any chance this gets merged? It would be super useful.

flovilmart · 2020-08-12T13:56:54Z

@icole what what left for update? I wanna go ahead with this. Can you pin point in the code the different changes required for the merge?

ethagnawl · 2020-08-12T15:02:22Z

From what I recall, it's a minor Code Climate nitpick. Unfortunately, the build is old enough that it's been dropped by CC. Could you try pushing a new commit in order to trigger a new build, @flovilmart?

flovilmart · 2020-08-13T17:19:45Z

@ethagnawl the problem is that the start method now is too complex, not sure how to simplify it given it's already quite simple. Any suggestion?

ethagnawl · 2020-08-13T18:23:19Z

I haven't used Code Climate in a long time and don't remember how some of their metrics work. I'd say maybe removing the project variable and nesting those method calls would work, but the more likely/correct solutions would be extracting those conditionals or moving the has creation into its own method.

Does anyone else have any thoughts about this or the PR in general? CC/ @tmuxinator/tmuxinator

flovilmart · 2020-09-08T02:00:59Z

@ethagnawl , finally, codeclimate passed! 🎉

ethagnawl · 2020-09-08T15:45:59Z

Okay, that's great. I'll have to find some time to review this again.

Sorry for the bother, but does anyone else have any thoughts? /CC @tmuxinator/tmuxinator

flovilmart · 2020-09-09T00:01:35Z

thanks @ethagnawl !

ethagnawl · 2020-09-22T17:16:13Z

lib/tmuxinator/config.rb

@@ -25,6 +25,10 @@ def home?
        File.directory?(home)
      end

+      def options_file
+        directory + ".yml"


The "yaml" extension is also supported. I tried using a project config file with this extension and everything worked as expected. Does that imply that this method isn't being used or am I overlooking something?

EDIT: After reading through this again, I don't think this could ever work. (Again, I may be missing something.)

what do you suggest?

Either this method needs to support both extensions or, if I'm correct in that it's not being used, it should be removed.

ethagnawl · 2020-09-22T17:26:15Z

lib/tmuxinator/project.rb

+      base = 0
+      if in_current_session
+        # Get the last window index + 1
+        base = 1 + `tmux list-windows | awk '{print $1}'`.split.last.to_i


I think this awk logic should either be handled in Ruby code or we should add a method to the Doctor class which verifies that awk exists on the user's system.

fair, let's me see what I can do

ethagnawl · 2020-09-22T17:49:24Z

lib/tmuxinator/cli.rb

+
+      def should_append?(options)
+        options[:append] == true ||
+          (options[:append] != false && Tmuxinator::Config.options[:append])


Could you walk me through the use case for the right side of this || and the related Config method?

the idea is that can set append in the Tmuxinator::Config.options so all sessions are appending by default.
if append is not provided by the options (case != false) we check that it is set in the global Config.

kurktchiev · 2020-12-02T13:42:08Z

any chance we can get this merged any time soon? it would really boost productivity with tmux/tmuxinator

cibinmathew · 2021-05-01T13:15:52Z

@ethagnawl @flovilmart @icole Thanks for this work.
Can we get this merged or may be list down the blocking issues and open another to track the enhancements. We could pitch in to get this to a closure.

ethagnawl · 2021-05-02T16:00:08Z

@ethagnawl @flovilmart @icole Thanks for this work.
Can we get this merged or may be list down the blocking issues and open another to track the enhancements. We could pitch in to get this to a closure.

There are still some issues (noted above) which need to be worked through. (If you feel like running with what's been done and @flovilmart doesn't mind, feel free to pick them up.) It'd also be good to get feedback from others in @tmuxinator/tmuxinator.

flovilmart · 2021-05-03T14:01:41Z

@ethagnawl let me get back on it

noahfrederick

Looking forward to this feature. I left a drive-by review of the code changes.

lib/tmuxinator/config.rb

lib/tmuxinator/cli.rb

lib/tmuxinator/project.rb

noahfrederick · 2021-06-02T13:55:29Z

lib/tmuxinator/window.rb

+      project_name = project.in_current_session ? "#{project.full_name}:" : ""
+      name ? "-n #{project_name}#{name}" : ""


I would prefer to drop this change, i.e., for the window to always be named without the prefix.

flovilmart · 2021-06-07T21:29:49Z

Thanks @noahfrederick will have a look later

ethagnawl · 2021-06-08T16:03:07Z

lib/tmuxinator/config.rb

@@ -56,6 +60,12 @@ def default
        "#{directory}/default.yml"
      end

+      def options
+        YAML.safe_load(File.read(options_file), [], [], true) || {}


@flovilmart Would you mind using the new safe_load method signature (remove positional parameters two, three and four and provide keyword argument for aliases)? As-is, this will cause errors in some environments because of changes to Psych and libyaml.

It would look something like this:

YAML.safe_load(File.read(options_file), aliases: true)

See this PR for more context.

megalithic · 2021-11-16T21:20:18Z

@flovilmart / @ethagnawl -- just pinging those that have worked on this. How are we feeling about your concerns being addressed and getting this merged?

flovilmart · 2021-11-16T22:53:13Z

@megalithic I haven't had the chance to actually finish this one; yeah I know, that's kinda pathetic after all this time 🤣

ethagnawl · 2021-11-17T14:27:56Z

@flovilmart I know your comment was in jest, but it's not pathetic at all. Your contributions are being provided at the expense of whatever else you might be doing with your life and are greatly appreciated.

megalithic · 2021-11-17T14:29:24Z

@ethagnawl @flovilmart 💯 agreed! OSS is a gift! i'm grateful for each of you that contribute/create such amazing tools.

flovilmart · 2021-11-20T17:13:48Z

Haha! Thanks y’all! very appreciated. I need to find some time for this. I see this is pretty requested.

ChrisYin · 2021-12-14T04:14:10Z

I'm wondering when this patch can be merged? Looks like it has a long history from 2018.

martin-braun · 2024-01-15T14:15:29Z

@flovilmart I know this issue probably hunts you in your dreams already, is there still any chance that you find the time to finish this?

akofink · 2024-03-31T19:51:51Z

I attempted to rebase this with tmuxinator:master, but I accidentally pushed the latest master, which auto-closed the PR. I have an updated branch that I will use to re-open a new PR, since I can no longer push to this original branch. Apologies!

flovilmart force-pushed the master branch from c0df62f to 87aabaf Compare June 25, 2019 14:53

flovilmart changed the title ~~Adds support for tmuxinator append~~ Adds support for tmuxinator start --append Jun 25, 2019

ethagnawl mentioned this pull request Aug 7, 2020

Being able to create projects, but for windows and panes #784

Open

ethagnawl reviewed Sep 22, 2020

View reviewed changes

noahfrederick reviewed Jun 2, 2021

View reviewed changes

ethagnawl reviewed Jun 8, 2021

View reviewed changes

ethagnawl mentioned this pull request Dec 10, 2021

best way to avoid code duplicate between project #840

Open

akofink added the feature label Feb 3, 2024

akofink closed this Mar 31, 2024

akofink force-pushed the master branch from 0c99b22 to 680b970 Compare March 31, 2024 19:49

akofink mentioned this pull request Mar 31, 2024

feat: Add support for tmuxinator append #908

Open

		project_name = project.in_current_session ? "#{project.full_name}:" : ""
		name ? "-n #{project_name}#{name}" : ""

Adds support for tmuxinator start --append #652

Adds support for tmuxinator start --append #652

Conversation

flovilmart commented Sep 16, 2018

ethagnawl commented Sep 19, 2018

flovilmart commented Sep 19, 2018

DestyNova commented Sep 19, 2018

flovilmart commented Sep 19, 2018

DestyNova commented Sep 19, 2018

flovilmart commented Sep 19, 2018

DestyNova commented Sep 19, 2018

ethagnawl commented Oct 30, 2018

flovilmart commented Oct 30, 2018

flovilmart commented Nov 10, 2018

raiyuza commented Feb 8, 2019

flovilmart commented Feb 8, 2019

icole commented Apr 5, 2019

flovilmart commented Apr 5, 2019

icole commented Apr 5, 2019

flovilmart commented Apr 5, 2019

flovilmart commented Jun 26, 2019

icole commented Jun 28, 2019

betoharres commented Feb 15, 2020

flovilmart commented Feb 15, 2020

protex commented Jul 22, 2020

flovilmart commented Aug 12, 2020

ethagnawl commented Aug 12, 2020

flovilmart commented Aug 13, 2020 • edited

ethagnawl commented Aug 13, 2020

flovilmart commented Sep 8, 2020

ethagnawl commented Sep 8, 2020

flovilmart commented Sep 9, 2020

ethagnawl Sep 22, 2020 • edited

Choose a reason for hiding this comment

flovilmart Jan 4, 2021

Choose a reason for hiding this comment

ethagnawl May 2, 2021

Choose a reason for hiding this comment

ethagnawl Sep 22, 2020

Choose a reason for hiding this comment

flovilmart Jan 4, 2021

Choose a reason for hiding this comment

ethagnawl Sep 22, 2020 • edited

Choose a reason for hiding this comment

flovilmart Jan 4, 2021

Choose a reason for hiding this comment

kurktchiev commented Dec 2, 2020

cibinmathew commented May 1, 2021

ethagnawl commented May 2, 2021

flovilmart commented May 3, 2021

noahfrederick left a comment

Choose a reason for hiding this comment

noahfrederick Jun 2, 2021

Choose a reason for hiding this comment

flovilmart commented Jun 7, 2021

ethagnawl Jun 8, 2021 • edited

Choose a reason for hiding this comment

megalithic commented Nov 16, 2021

flovilmart commented Nov 16, 2021

ethagnawl commented Nov 17, 2021

megalithic commented Nov 17, 2021

flovilmart commented Nov 20, 2021

ChrisYin commented Dec 14, 2021

martin-braun commented Jan 15, 2024

akofink commented Mar 31, 2024 • edited

flovilmart commented Aug 13, 2020 •

edited

ethagnawl Sep 22, 2020 •

edited

ethagnawl Sep 22, 2020 •

edited

ethagnawl Jun 8, 2021 •

edited

akofink commented Mar 31, 2024 •

edited