✨ zb: Add a function to create a builder for IBus#1040
✨ zb: Add a function to create a builder for IBus#1040SummerGram wants to merge 4 commits intoz-galaxy:mainfrom SummerGram:add-ibus
Conversation
zeenix
left a comment
zeenix
left a comment
There was a problem hiding this comment.
Thanks so much. It looks good for a first-time contributions but it will need a bit of work still. Would be great to also add a test case and enable that in the CI (only for the Linux part). You'll probably need to try and install the appropriate ibus package.
|
Oh and I really like that you added an emoji as per our practices. However a 💥 isn't that best choice here since it's not a breaking change. |
Haha! I just follow the repo practice. I tried to add long and detailed message in a commit but not work. |
SummerGram
changed the title
Cool. Can you tell me how your'e choosing/selecting the emojis? The |
SummerGram
changed the title
I just go to web and pick emoji. Now I install |
|
I rebase it and stash it. Please comment it |
|
@SummerGram Oh, nm my previous comment (now deleted). I forgot that you were asked to rebase on #325. Unfortunately, that branch/PR needs rebasing too now. I'll ask the author if they'd still be moving the PR forward.. |
|
I see. I guess that PR will merge to the current main. And what I do is rebase to that new merged main. Please tell me when it is merged here or in matrix. I do not check that PR often. |
Yes. :)
Sure thing! |
|
@SummerGram seems you're not around on Matrix so just to let you know #976 has been merged. Please rebase and address the comments so we could get this merged too. 🙏 |
Mind if I create a new PR? |
Even if you start fresh, you can just force push to the same branch so there's no need. It'll only create noise. |
|
Can you please check the base branch correct or not? |
Given that the PR now only has commits from your iBus work, I think it's a pretty safe bet the base is now the I'll have a look soon but please make sure that commits that are fixups to previous commits (in the same PR/branch), are squashed into those previous commits. |
Create a helper function in `conn::builder` to build a connection to IBues daemon Fixes #964
zeenix
left a comment
zeenix
left a comment
There was a problem hiding this comment.
Getting very close, I think. Apart from these minor things, you need to check the CI. The error is very strange since ibus address on my machine does have a : in it, as it should.
| #[cfg(not(feature = "tokio"))] | ||
| let output = child_process | ||
| .output() | ||
| .await | ||
| .expect("Fail to run `ibus address`"); | ||
|
|
||
| #[cfg(feature = "tokio")] | ||
| let output = child_process | ||
| .wait_with_output() | ||
| .await | ||
| .expect("Fail to run `ibus address`"); |
There was a problem hiding this comment.
Sorry, just noticed this part. Why do they need to be different? tokio::process::Command also has output method, which should be good enough for us?
Actually, abstractions::process::Command::output can do most stuff already for us here? 🤔 You don't even need to bother with piping etc.
Did you try to fix this? |
2 participants
ibusinconn::builderwhich callsibus addressand then create the builderFixs #964.