Closed golden-expiriensu closed 4 months ago
Resolves https://github.com/altsem/gitu/issues/37 btw
Yea, TargetOp was a mess, I've been trying to make this interface better. And now it's gone. Apologize for the conflicts :man_facepalming:. I'll have a look and see if I can help :+1:
This is how it looks like in master right now:
pub(crate) type Action = Rc<dyn FnMut(&mut State, &mut Term) -> Res<()>>;
pub(crate) trait OpTrait: Display {
/// Get the implementation (which may or may not exist) of the Op given some TargetData.
/// This indirection allows Gitu to show a contextual menu of applicable actions.
fn get_action(&self, target: Option<&TargetData>) -> Option<Action>;
/// This indicates whether the Op is meant to read and
/// act on TargetData. Those are listed differently in the help menu.
fn is_target_op(&self) -> bool {
false
}
}
Starting to feel the complexity vs the gains of doing this is kind of not worth it.
Perhaps should be changed back to just be a trigger
function, and nothing more.
pub(crate) trait OpTrait: Display {
fn trigger(&self, target: Option<&TargetData>);
}
This means you would lose the contextual "target" column (here ret Show
is listed with the other ops):
For now, you could emulate this by just doing:
fn get_action(&self, _target: Option<&TargetData>) -> Option<Action> {
Some(Rc::new(|state: &mut State, _term: &mut Term| {
...
Ok(())
}))
}
I am doing a rebase right now and actually new updates are looking pretty good so far, so its okay, no worries
@altsem Sorry for stash_push_action_prompt_update
arguments, I wanted to accept iterator over static strings, but lost to lifetimes :(
Very cool! Having a look :)
Ooh, thank you very much! I actually managed to solve this issue with stash_push_action<const N: usize>(args: [&'static str; N])
. So pick which variant you would like to keep in the repo and I will update code accordingly
Attention: Patch coverage is 15.19608%
with 173 lines
in your changes are missing coverage. Please review.
Project coverage is 78.75%. Comparing base (
8069c38
) to head (f850cdc
).
Files | Patch % | Lines |
---|---|---|
src/ops/stash.rs | 0.00% | 123 Missing :warning: |
src/items.rs | 35.00% | 26 Missing :warning: |
src/screen/status.rs | 50.00% | 16 Missing :warning: |
src/ops/mod.rs | 12.50% | 7 Missing :warning: |
src/ops/show.rs | 0.00% | 1 Missing :warning: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Looks good so far! Just some tests missing. Let me know if you need help with this, I wouldn't mind writing them either @golden-expiriensu.
Then when you feel it's ready, let me know and I'll have a final look / test things out manually too.
Haven't settled on a strict commit format, I'll update the PR name just in case I forget. The "feat: " prefix will result in a changelog entry.
Hey @altsem, I added tests for stash functionality, the PR is ready for the final review, let me know if I missed something P.S. Btw, the tooling and test setup is awesome, I had a great time writing it
Attention: Patch coverage is 97.90698%
with 9 lines
in your changes are missing coverage. Please review.
Project coverage is 85.86%. Comparing base (
2f8846b
) to head (75d2af0
).
Files | Patch % | Lines |
---|---|---|
src/items.rs | 87.50% | 5 Missing :warning: |
src/ops/stash.rs | 97.56% | 3 Missing :warning: |
src/ops/show.rs | 0.00% | 1 Missing :warning: |
:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
@golden-expiriensu awesome :)
Tested it out quickly, there's an issue with StashWorktree though. I had a pre-commit hook causing the commit to fail, and rest to mess up my state.
I saw there's this option --keep-index:
git stash push --keep-index --include-untracked
~Does this solve it all?~ Right, --keep-index includes the index in the stash actually..
Oooh indeed, I completely forgot about hooks. I will try to find workarounds for that
Another note @golden-expiriensu. Perhaps it is possible to do the commit-dance with the stash instead?
It'd be more robust if the reset was not relative. Could do something like:
let previous_commit_hash = *get the head*?;
let result = *do things*;
git reset --soft *previous_commit_hash*?;
result? // Bubble the result afterwards, just in case something fails
@altsem of course we can! I don' t know why I decided to use commits instead of stashes in the first place
@altsem done, it now uses stash as a temporary storage of index when stashing working tree, PTAL
I read the code and had another go at testing it. Pretty happy with the result! Found some interesting differences between cli git and magit. It seems stashing things with magit will "remember" which things are staged and not.
Gj! I'll merge this! :rocket:
Almost everything is working as expected, but unfortunately I have no idea so far how to